-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 Mobile #11618
feat: Sort/Import Tokens in Mobile #11618
Conversation
…/metamask-mobile into chore/componetize-tokens-screen
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. |
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.
a couple of very minor code optimisations
i just tested the feature , looks great , lgtm ✅ |
…-token-sort-config
I ve seen this sonar issue https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=11618&id=metamask-mobile&open=AZJ4jkkCJ8jX2B2IGuL8 I think its related to this signature being deprecated I have seen a thread about this suggesting to use MetricsEventBuilder and non-deprecated trackEvent instead: |
Bitrise🔄🔄🔄 Commit hash: f376551 Note
|
@sahar-fehri have you pulled the latest? This is what I see: |
Bitrise✅✅✅ Commit hash: e106631 Note
|
Quality Gate passedIssues Measures |
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.
LGTM. Only nit - The term Declining Balance
along with the red highlight makes it seem like a destructive action, which is not since sorting is for display only. To keep it neutral and consistent with the other Alphabetically
, I'd recommend using the word Balance
followed by high to low and remove the red highlight.
This is a good callout. I should have cut a new recording since this was addressed: Screen.Recording.2024-10-22.at.12.02.40.PM.movAs an aside, I have a follow up PR to update this to a BottomSheet component anyway, so should be pretty short-lived |
Description
Implements Token Sorting on mobile. This is the mobile implementation of the same feature on extension: MetaMask/metamask-extension#27184
Note that this currently includes a patch of the
PreferencesController
. There is a corresponding PR in core to add this to a formal controller release: MetaMask/core#4747Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-357
Manual testing steps
Go to Portfolio Screen
Sort should allow sorting by declining fiat balance by default. Users can then toggle and sort alphabetically or by declining fiat balance.
Import button should function the same as the import token link in footer
Screenshots/Recordings
Screen.Recording.2024-10-03.at.10.37.36.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist