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: Add tokenSortConfig to preferences controller #4747

Closed
wants to merge 8 commits into from

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Oct 1, 2024

Explanation

Adds a tokenSortConfig to PreferencesController to pair with mobile and extension codebases. This config will tell each application which keys tokens are sorted by, which order they are sorted in, and by which sorting callback they should use while being sorted.

References

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

Changelog

@gambinish gambinish requested a review from a team as a code owner October 1, 2024 06:05
@gambinish gambinish requested a review from a team as a code owner October 4, 2024 16:08
@sahar-fehri
Copy link
Contributor

Looks good!
Can we update change log to reflect the update; sth like

@metamask/preferences-controller

  • Added: Added tokenSortConfig to preferencesController state

github-merge-queue bot pushed a commit to MetaMask/metamask-mobile that referenced this pull request Oct 22, 2024
## **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#4747

## **Related 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**


https://github.com/user-attachments/assets/f9a9cf52-ef23-4705-a7db-ae5ecdc232f7

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.

---------

Co-authored-by: salimtb <salim.toubal@outlook.com>
@sahar-fehri
Copy link
Contributor

@cryptodev-2s
Copy link
Contributor

cryptodev-2s commented Oct 30, 2024

Closed as change was included in #4860

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.

3 participants