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

feat: Sort/Import Tokens in Extension #27184

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Sep 17, 2024

Description

This PR adds Token sorting to the Asset List page, and also moves Token importing to the top of the Token List. A few of the main changes introduced:

  1. Include NativeToken in TokenList component to be included in sorting logic, and treated (as far as sorting is concerned) as any other token in the list
  2. Intoduce a tokenSortConfig into state that keeps track of the sort order, the key being sorted by, and the direction of the sort order. Also includes an action to update this state.
  3. Introduce a useEffect that subscribes to tokenSortConfig as well as a few other application state variables to update and sort tokenList when appropriate.
  4. Clean up asset-list component, and move some of it's relevant code into the useAccountTotalFiatBalance

Acceptance Criteria:

  1. Tokens should be sorted by default by declining balance
  2. Sort controls should sort tokens alphabetically, and by decreasing fiat token balance
  3. Sort order should persist through refresh
  4. Sort order should persist after app is closed and reopened
  5. When a token gets imported, it should be included in the sort list, in the correct order in the list

A couple of disclaimers. There are still (at least) two bugs that I discovered that were not caught by tests:

  1. When toggling preferred currency setting, Native Token sorted incorrectly by decreasing fiat balance ✅ fixed
  2. When switching between accounts, token list does not update ✅ fixed

Open in GitHub Codespaces

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-356

Manual testing steps

  1. Go to AssetList page, and click dropdown and select option to sort by
  2. Tokens should sort, and remain sorted through refresh and application close/open (it is in state)
  3. Importing tokens should import them into the sort order

Screenshots/Recordings

Screen.Recording.2024-09-16.at.7.57.32.PM.mov

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.

gambinish and others added 30 commits September 5, 2024 12:23
…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension
…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension
…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension
@metamaskbot
Copy link
Collaborator

Builds ready [e7a879e]
Page Load Metrics (2543 ± 485 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint901447023971033496
domContentLoaded1433438625121013486
load1475439725431011485
domInteractive12382818038
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.38 KiB (0.03%)
  • ui: 17.01 KiB (0.22%)
  • common: 571 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [f4ef0da]
Page Load Metrics (2007 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint170924412010233112
domContentLoaded169923131973208100
load170824182007224107
domInteractive15261515828
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.38 KiB (0.03%)
  • ui: 17.01 KiB (0.22%)
  • common: 571 Bytes (0.01%)

bergeron
bergeron previously approved these changes Oct 7, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [0cd3e25]
Page Load Metrics (1840 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29724651703466224
domContentLoaded16042391180316981
load16152463184018589
domInteractive268647209
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.38 KiB (0.03%)
  • ui: 17.01 KiB (0.22%)
  • common: 571 Bytes (0.01%)

Copy link

sonarcloud bot commented Oct 8, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [c8467f6]
Page Load Metrics (1934 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26722921771510245
domContentLoaded16272245190715775
load16612280193416479
domInteractive2910156199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.38 KiB (0.03%)
  • ui: 17.01 KiB (0.22%)
  • common: 571 Bytes (0.01%)

@gambinish gambinish added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@gambinish gambinish added this pull request to the merge queue Oct 8, 2024
Merged via the queue into develop with commit b34484e Oct 8, 2024
78 checks passed
@gambinish gambinish deleted the feat/mmassets-356_sort-import-tokens-extension--persistence-telemetry branch October 8, 2024 22:42
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 8, 2024
@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants