fix: label main and secondary balances #13462
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
bergarces
force-pushed
the
fix/MMASSETS-537-mobile-fiat-native-toggle
branch
6 times, most recently
from
d98d06f to
7e60898
Compare
|
|
||
| interface AssetElementProps { | ||
| children?: React.ReactNode; | ||
| asset: TokenI; | ||
| onPress?: (asset: TokenI) => void; | ||
| onLongPress?: ((asset: TokenI) => void) | null; | ||
| balance?: string; | ||
| mainBalance?: string | null; |
There was a problem hiding this comment.
This is the least disruptive way of fixing the nomenclature without altering behaviour of different components inadvertently.
mainBalance, despite its name, was added after balance and was being used as a secondary balance. By renaming this one and keeping the main balance as balance, it becomes consistent without major changes.
| @@ -1,2 +1,2 @@ | |||
| export const FIAT_BALANCE_TEST_ID = 'fiat-balance-test-id'; | |||
| export const MAIN_BALANCE_TEST_ID = 'main-balance-test-id'; | |||
There was a problem hiding this comment.
Renaming these test ids for consistency. Snapshots will need updating accordingly.
| @@ -101,6 +101,7 @@ const initialState = { | |||
| }, | |||
| settings: { | |||
| showTestNetworks: true, | |||
| primaryCurrency: 'ETH', | |||
There was a problem hiding this comment.
Need this to maintain test descriptions and assertions accurate, as otherwise the default is fiat and the TokenListItem component has now been fixed.
| @@ -207,7 +207,7 @@ const StakingBalanceContent = ({ asset }: StakingBalanceProps) => { | |||
| {hasEthToUnstake && !isLoadingPooledStakesData && ( | |||
| <AssetElement | |||
| asset={asset} | |||
| mainBalance={stakedBalanceETH} | |||
| secondaryBalance={stakedBalanceETH} | |||
| balance={stakedBalanceFiat} | |||
There was a problem hiding this comment.
This component is owned by @MetaMask/metamask-staking and is currently displaying fiat balance as the primary and ETH balance as secondary without taking into account the primaryCurrency setting.
Since this is a bug fix for another component, I didn't want to change the behaviour of this one, but it's probably worth checking what it should be.
bergarces
force-pushed
the
fix/MMASSETS-537-mobile-fiat-native-toggle
branch
from
7e60898 to
f4773c3
Compare
| @@ -144,8 +144,8 @@ const Balance = ({ asset, mainBalance, secondaryBalance }: BalanceProps) => { | |||
| </Text> | |||
| <AssetElement | |||
| asset={asset} | |||
| mainBalance={mainBalance} | |||
| balance={secondaryBalance} | |||
There was a problem hiding this comment.
The ticket does not specify that we ought to change the behaviour of this component, but I have, given that the same issue applied here.
The way it determines mainBalance and secondaryBalance looks a bit odd (here)
If portfolio view is enabled, it always sets mainBalance as the chain's native currency and secondaryBalance as the fiat.
If portfolio view is NOT enabled, then it sets the mainBalance and secondaryBalance in accordance to the primaryCurrency setting.
They were however being passed the wrong way around, which meant that secondaryBalance was being displayed as the main balance and mainBalance was being displayed as the secondary balance. It should now display as intended.
bergarces
force-pushed
the
fix/MMASSETS-537-mobile-fiat-native-toggle
branch
from
f4773c3 to
62f8e82
Compare
|
|
Description
It's been reported that setting the primary currency to "Native" was displaying fiat as the main currency and the native currency as secondary. The opposite would happen when setting the primary currency to "Fiat".
This was due to using the wrong nomenclature as the properties of the component responsible for that behaviour (
balancewas the primary balance andmainBalancewas the secondary balance).The least disruptive behaviour that fixes both the issue and the nomenclature is leaving
balanceas the primary balance changing the name ofmainBalancetosecondaryBalance.This component is used in three different places:
The behaviour for the token list and balance have been fixed to work as the setting suggests. The staking balance has been left as it currently is pending confirmation of whether it needs to follow the same behaviour.
Related issues
Fixes: No open GitHub issue for the ticket
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist