3rd Update - Switch NetworkDisplay with PickerNetwork #21083
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. |
MaxwellDG
changed the title
| as="div" | ||
| src={currentNetwork?.rpcPrefs?.imageUrl} | ||
| label={ | ||
| providerConfig?.type === NETWORK_TYPES.RPC | ||
| ? providerConfig?.nickname ?? t('privateNetwork') | ||
| : t(getNetworkLabelKey(providerConfig?.type)) | ||
| } | ||
| backgroundColor={BackgroundColor.transparent} | ||
| borderColor={BorderColor.borderMuted} | ||
| iconProps={{ display: Display.None }} |
There was a problem hiding this comment.
@MaxwellDG @georgewrmarshall Do we have icons for our built in networks? I don't know if we want to introduce the letters here? Might cause user confusion.
There was a problem hiding this comment.
I don't believe there's icons for the built-in ones. The previous components just used small coloured circles. And I couldn't see any Sepolia or Goerli related images in Storybook. Maybe you know of some elsewhere you'd like me to use?
There was a problem hiding this comment.
In ui\selectors\selectors.js, where the data for the test networks are retrieved from, this is an example object:
{ chainId: CHAIN_IDS.GOERLI, nickname: GOERLI_DISPLAY_NAME, rpcUrl: CHAIN_ID_TO_RPC_URL_MAP[CHAIN_IDS.GOERLI], providerType: NETWORK_TYPES.GOERLI, ticker: TEST_NETWORK_TICKER_MAP[NETWORK_TYPES.GOERLI], id: NETWORK_TYPES.GOERLI, removable: false, },
Doesn't look like they have the property .rpcPrefs?.imageUrl which usually supplies the icon for other networks
There was a problem hiding this comment.
I believe the icons are retrieved here
src={currentNetwork?.rpcPrefs?.imageUrl}
Sepolia and Goreli test networks use a combination of html elements and color to create the icon but this adds complexity to how we display networks. I'll create a ticket to add the images and migrate them over. This should reduce the complexity if all networks are displayed using images.
There was a problem hiding this comment.
Created an issue here #21142
There was a problem hiding this comment.
The code is looking good but I'm not sure this component is used anywhere so instead of replacing the NetworkDisplay component we may be able to remove this folder(ui/components/app/signature-request-header) completely. Apologies for not catching this earlier. The good news is by removing these component files we will get some small performance benefits in prod and dev because we are also removing redundant CSS, tests and storybook files. Let's get confirmation from @matthewwalsh0 on the confirmations team to see if we can remove them.
georgewrmarshall
added
team-design-system
|
Hey @MaxwellDG, Can you update the snapshots for this PR too? |
MaxwellDG
force-pushed
the
update/picker-network-3
branch
from
397a527 to
5623313
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #21083 +/- ##
===========================================
- Coverage 68.38% 68.38% -0.00%
===========================================
Files 1052 1052
Lines 41888 41892 +4
Branches 11164 11167 +3
===========================================
+ Hits 28645 28646 +1
- Misses 13243 13246 +3 ☔ View full report in Codecov by Sentry. |
This is fixing a 3nd instance of #20485.
As stated by @georgewrmarshall in #20485 there are some expected, and accepted UI differences:
The colourful circle now includes a letter when it's not an image
The height is slightly different. They're now all static at 32px
Initial:
Current:
