Add use effect on Balance component for the price

[carletex]

The usdMode flag wasn't working on our Balance component, since the price takes some time to load and we were always setting displayUsdMode to false on the first render (price > 0 condition)

You can test in page.tsx

/* This will always show the Balance in ETH mode, even if we say usdMode */
<Balance address={connectedAddress} usdMode /> 

This PR adds the missing useEffect (as in EtherInput).

[rin-st]

Fix works great!

But I think we need to consider rewriting a bit the logic of the Balance and EtherInput components (in another pull request). Because usdMode prop is basically doesn't mean real mode inside the component because of internal toggling logic. I'm thinking about two options

1. Rename usdMode to defaultUsdMode and use that value as default in useState. Remove useEffect. (I prefer this)
2. Remove inner usd/eth logic. Use only usdMode prop (or mode)

Yes, with that options we can't change mode of let's say group of EtherInputs, but not sure we need it.

Also, who remembers why we added that price > 0 condition? Looks like it's redundant

[rin-st]

I think it's ok to merge it for usdMode fix, but as I said looks like components need updates

[carletex]

Thanks for the review @rin-st !

who remembers why we added that price > 0 condition? Looks like it's redundant

The USD mode only makes sense when we have a price for the native currency. If it's loading (or if the fetch fails) it doesn't make sense to show the USD mode... since you won't be able to convert to the native currency (which is what we want for any onchain operation / show balance, etc). So I think that was the reason.

Rename usdMode to defaultUsdMode and use that value as default in useState. Remove useEffect. (I prefer this)

This will only work if we ignore my previous comment (the price condition). I agree that defaultUsdMode might be a better name.

Remove inner usd/eth logic. Use only usdMode prop (or mode)

I don't like this one since it'd require people to do their logic for the basic toggle USD / ETH.

---

I'm not sure if these components need a refactor, but like you said, it should probably happen in another PR.

I just created this one as a hotfix, to tackle a bug that is currently happening on main (and tried to be consistent with other components)

[rin-st]

Thank you for explanation!

I just created this one as a hotfix, to tackle a bug that is currently happening on main (and tried to be consistent with other components)

Agree, lgtm!

[technophile-04]

Thanks @carletex for the quick fix !! Merging this

---

For another PR :

Agree on naming that it should be called defaultUdMode

Regarding working, wouldn't it be more intuitive if we had this flow, just thinking out loud :

1. Initial state of price (global store is undefined) [denoting we haven't made any request yet for price]
2. The initial state of displayUsdMode should be only derived from defaultUdMode and not the price.
   • const [displayUsdMode, setDisplayUsdMode] = useState(defaultUsdMode);
3. We should have an extra global loading state (isFetchingPrice) which denotes price is being fetched
4. The loading skeleton will be shown if isFetchingPrice || isBalanceInETHLoading ... are true
5. An useEffect which checks if price === 0 && defaultUsdMode then setDisplayUsdMode(false) [With maybe a console.error("Error getting price") ]

I think isFetchingPrice is good to have in global store since price is derived from async action and people would love to have it exposed to workaround the flickering / layout shift

[rin-st]

Thanks for your answer Shiv, makes sense. But I don't understand how component will work ifdefaultUsdMode prop changes? I think we need to tinker with that component and find the best solution in the process. I see one problem with naming: If prop named defaultUsdMode it should affect only default inner value, and not change it later if prop value changes. (See point 1) #856 (comment) For me with current logic it's better to leave prop name as is.