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

chore: update and fix bugs in asset-picker network modal #28416

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

micaelae
Copy link
Member

@micaelae micaelae commented Nov 12, 2024

Description

Change Before After
NetworkListItem + AssetPickerModalNetwork
add shouldDisableNetwork prop that determines whether a visible network should be selectable
N/A Screenshot 2024-11-12 at 10 21 30 AM
Figma: dest network picker
AssetList
fix: duplicate component keys cause unexpected search behavior when there are multiple tokens with the same symbol
Screenshot 2024-11-12 at 10 45 45 AM Screenshot 2024-11-12 at 10 59 56 AM
AssetList
hide native asset balance when displaying tokens for an inactive network and prevents wrapping balance line
Screenshot 2024-11-12 at 10 46 03 AM Screenshot 2024-11-12 at 10 59 26 AM
AssetPickerModal
isTokenListLoading prop displays a loading overlay until the token list is available. The cross-chain swaps experience fetches token lists from the bridge-api, which can take a few seconds after changing the network. Without a loading screen, the user is able to select a token from the previously selected network, which may be invalid in the new network
WLD does not exist in the avalanche network but is selected: Screenshot 2024-11-12 at 11 02 59 AM
Video
Screenshot 2024-11-12 at 11 11 25 AM
Video
AssetPickerModal
change the network picker label to default to NETWORK_TO_NAME_MAP instead of the network name for consistency
Screenshot 2024-11-12 at 10 49 59 AM Screenshot 2024-11-12 at 10 56 38 AM
AssetPickerModalNetwork
add header prop for displaying a custom header and implement search functionality
Screenshot 2024-11-12 at 10 50 24 AM Screenshot 2024-11-12 at 10 55 54 AM Screenshot 2024-11-12 at 10 55 45 AM
AssetPicker
add a prop for overriding the AssetPicker trigger button with a different component
Screenshot 2024-11-12 at 10 50 37 AM bridge asset picker button

Open in GitHub Codespaces

Related issues

Fixes: N/A

Manual testing steps

The network picker changes can only be tested locally and if other cross-chain swaps changes are pulled into the branch. The only change that could affect the Swap+Send experience is fixing the AssetList keys

  1. Select Polygon as the wallet's network
  2. Open Send experience
  3. Click on src token
  4. Search for "CSM"
  5. Verify that only 2 CSMATIC tokens appear and console errors related to keys are no longer visible

Screenshots and Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 12, 2024
@micaelae micaelae changed the title chore: update asset-picker and fix network picker bugs chore: update and fix bugs in asset-picker network modal Nov 12, 2024
@micaelae micaelae marked this pull request as ready for review November 12, 2024 21:13
@micaelae micaelae requested review from a team as code owners November 12, 2024 21:14
@metamaskbot
Copy link
Collaborator

Builds ready [2eb3e4c]
Page Load Metrics (1858 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16652294186316780
domContentLoaded16272214182415172
load16652295185816680
domInteractive177444178
backgroundConnect779332612
firstReactRender5311797157
getState4231063
initialActions01000
loadScripts11601558133211555
setupStore55916189
uiStartup18302609206518689
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.51 KiB (0.02%)
  • common: 50 Bytes (0.00%)

};

export default function AssetList({
handleAssetChange,
asset,
tokenList,
isTokenDisabled,
network,
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we not require a network here?

Copy link
Member Author

Choose a reason for hiding this comment

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

When the cross-chain swaps experience is initially loaded a network won't be set for the destination (Bridge to) asset picker: https://www.figma.com/design/IuOIRmU3wI0IdJIfol0ESu/Cross-Chain-Swaps?node-id=159-28072&m=dev

@metamaskbot
Copy link
Collaborator

Builds ready [b22c344]
Page Load Metrics (1951 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint51524671884369177
domContentLoaded16712410191017885
load16802495195119091
domInteractive267751189
backgroundConnect792452411
firstReactRender532971126330
getState493232412
initialActions00000
loadScripts11751741139413967
setupStore65612136
uiStartup188427412187238115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.65 KiB (0.02%)
  • common: 98 Bytes (0.00%)

@gambinish gambinish self-requested a review November 12, 2024 23:28
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

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

LGTM.

We'll have some conflicts on the Multichain AssetList, but I can address them there.

@micaelae micaelae removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants