-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(4057): replace asset avatars with network avatars #13910
base: main
Are you sure you want to change the base?
feat(4057): replace asset avatars with network avatars #13910
Conversation
…n list. added new hook to fetch aggregated chain data for all accounts
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. |
…alancse for account selector list are always shown now
|
…4057-replace-asset-avatars-with-network-avatars
|
@@ -27,51 +27,55 @@ const AvatarGroup = ({ | |||
maxStackedAvatars = DEFAULT_AVATARGROUP_MAXSTACKEDAVATARS, | |||
includesBorder = true, | |||
spaceBetweenAvatars, | |||
renderOverflowCounter = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is introduced to optionally avoid rendering the overflow counter to match the Figma
with prop | without prop |
---|---|
![]() |
![]() |
expect( | ||
within(businessAccountItem).getByText(regex.usd(3200)), | ||
).toBeDefined(); | ||
|
||
expect(within(businessAccountItem).queryByText('••••••')).toBeNull(); | ||
}); | ||
}); | ||
it('Text is hidden when privacy mode is on', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is removed because we are no longer showing the selected network token balance, instead it shows the network avatar icons. See the before and after screenshots
const fiatBalanceStrSplit = fiatBalance.split('\n'); | ||
const fiatBalanceAmount = fiatBalanceStrSplit[0] || ''; | ||
const tokenTicker = fiatBalanceStrSplit[1] || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer shows the token balance based on selected network. See before and after screenshots
@@ -73,7 +78,7 @@ const useAccounts = ({ | |||
); | |||
const formattedTokensWithBalancesPerChain = useGetFormattedTokensPerChain( | |||
internalAccounts, | |||
!isTokenNetworkFilterEqualCurrentNetwork, | |||
shouldAggregateAcrossChains || !isTokenNetworkFilterEqualCurrentNetwork, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This arg is added to allow the opting out of the popular network filter. Going forward we should always show the aggregated balance of each account in the app/components/UI/AccountSelectorList/AccountSelectorList.tsx
. The network filter should not affect the AccountSelectorList
component and should only affect the token list
|
|
|
avatarGroup: { | ||
borderRadius: 4, | ||
borderWidth: 2, | ||
height: 16, | ||
width: 16, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these styles be applied to all AvatarGroup
styles throughout the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres only one other instance of it being used which is network permission summary when interacting with dapps. I believe this change is fine for that scenario but I'll double check with Hester
Description
This PR changes the account list item to display network avatars. This change supports a more network-centric approach to account selection, which is more valuable in a multichain context. Here is the Figma
Implementation
useMultiAccountChainBalances
hook to get balance data across chains for all accountsRelated issues
Feature: #4057
Manual testing steps
Screenshots/Recordings
Recording
Screenshot
Before
NA
After
NA
Pre-merge author checklist
Pre-merge reviewer checklist