Skip to content
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

UX: Show Network Logo Based on Chain ID #20895

Merged
merged 3 commits into from
Sep 22, 2023
Merged

UX: Show Network Logo Based on Chain ID #20895

merged 3 commits into from
Sep 22, 2023

Conversation

darkwing
Copy link
Contributor

Explanation

Adding a network from within MetaMask (the 'Add Network' functionality within 'Settings') properly provides a {network}.rpcPrefs.imageUrl so that networks in the following areas show the correct network icon:

  1. Network picker
  2. Network menu
  3. Token badge icon
  4. NewNetworkInfo modal

The problem is that adding a network from a dapp (example: adding a network via Uniswap) doesn't always mean they provide an imageUrl.

This PR updates thegetNonTestNetworks selector so that it provides an imageURL or falls back to providing known network icons based on chain ID.

Screenshots/Screencaps

After

SCR-20230914-tjzx SCR-20230914-tjju SCR-20230914-tjgi

Manual Testing Steps

  1. Start with a new MetaMask wallet ( so that there aren't existing L2 networks)
  2. Go to https://app.uniswap.org/
  3. Go to the Swaps page
  4. Switch to a network via their network picker
  5. See MetaMask pop up, asking to add the new network
  6. Approve adding the new networks, which adds the network info without the imageUrl
  7. See: (1) The network picker and list show the correct icon, (2) the token list showing the correct network badge, and (3) switching to the network the first time shows the correct badge

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@darkwing darkwing requested a review from a team as a code owner September 15, 2023 03:31
@github-actions
Copy link
Contributor

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.

@darkwing darkwing added team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead needs-ux-ds-review labels Sep 15, 2023
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4d5c2be) 68.17% compared to head (27a8525) 68.17%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #20895   +/-   ##
========================================
  Coverage    68.17%   68.17%           
========================================
  Files          998      998           
  Lines        39963    39965    +2     
  Branches     10684    10686    +2     
========================================
+ Hits         27242    27244    +2     
  Misses       12721    12721           
Files Changed Coverage Δ
ui/selectors/selectors.js 86.67% <100.00%> (+0.04%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [8815613]
Page Load Metrics (1396 ± 14 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint106145123115
domContentLoaded1340145513952914
load1340145513962914
domInteractive1340145513952914
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 157 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham
Copy link
Contributor

I tested this using https://app.uniswap.org, and most screens showed the logo correctly, but this one did not

image

DDDDDanica
DDDDDanica previously approved these changes Sep 15, 2023
@darkwing
Copy link
Contributor Author

@HowardBraham ...could you try again? Optimism worked for me the first time, then I deleted that network, re-added it via Uniswap, and the icon displayed as it should

NidhiKJha
NidhiKJha previously approved these changes Sep 15, 2023
Copy link
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darkwing darkwing dismissed stale reviews from NidhiKJha and DDDDDanica via 27a8525 September 15, 2023 13:56
@metamaskbot
Copy link
Collaborator

Builds ready [27a8525]
Page Load Metrics (1823 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1292681623215
domContentLoaded15952116182113565
load15952116182313364
domInteractive15952116182113565
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 157 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham
Copy link
Contributor

@darkwing I tested it again on Firefox with a brand-new clean version of the Extension, no history.
Same result... but... I noticed it's only a problem on some networks!
Optimism and Arbitrum are broken, but Polygon, BNB, Avalanche, and Celo are all fine.
(Side note: we have the old Celo logo)

@darkwing
Copy link
Contributor Author

@HowardBraham I think we should consider that a separate bug, not only because the image is wrong, but also it states that "ETH" is the native currency of Optimism, etc.

@darkwing
Copy link
Contributor Author

Filed new issue here: #20934

@darkwing darkwing merged commit 55475df into develop Sep 22, 2023
11 checks passed
@darkwing darkwing deleted the ux-network-logo branch September 22, 2023 21:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.3.0 Issue or pull request that will be included in release 11.3.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants