-
-
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: Token Network Filter UI [Mobile] #11808
base: main
Are you sure you want to change the base?
Conversation
…/metamask-mobile into chore/componetize-tokens-screen
…Mask/metamask-mobile into feat/mmassets-431_network-filter-ui
Bitrise❌❌❌ Commit hash: fcafb86 Note
Tip
|
Bitrise❌❌❌ Commit hash: de35586 Note
Tip
|
Just leaving a comment per @Cal-L suggestion. The failing Bitrise workflow is related to some flaky Android tests that are currently being investigated. They are unrelated to this PR and it can be reviewed. |
Bitrise✅✅✅ Commit hash: 915549e Note
|
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.
Left some comments. Let's also remove the empty horizontal gutter from the pills.
app/components/UI/Tokens/styles.ts
Outdated
}, | ||
controlButtonText: { | ||
color: colors.text.default, | ||
...fontStyles.normal, |
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.
Use font from theme instead
useTokenDetection: { persist: true, anonymous: true }, | ||
smartTransactionsOptInStatus: { persist: true, anonymous: false }, | ||
useTransactionSimulations: { persist: true, anonymous: true }, | ||
+ useSafeChainsListValidation: { persist: true, anonymous: true }, | ||
+ showMultiRpcModal: { persist: false, anonymous: false }, | ||
+ tokenSortConfig: { persist: true, anonymous: false }, | ||
+ tokenNetworkFilter: { persist: true, anonymous: false }, |
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.
Has this logic been updated in the controllers already?
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.
Good call, it may be updated now, I'll check. I think only the tokenNetworkFilter
has not yet been released in core
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.
It looks like some of these were included in the latest release of PreferencesController. Are you okay with addressing this upgrade in a separate PR? I don't want to expand the scope of this any more than it needs to be.
I attempted an upgrade, but it appears there are a few unrelated things that also need to be updated to properly bump the version as we need it to.
33d6d30
to
915549e
Compare
Bitrise✅✅✅ Commit hash: fd28086 Note
|
Quality Gate passedIssues Measures |
Description
This PR introduces token network filter UI component. It lives behind a feature flag
PORTFOLIO_VIEW
in order to allow this to get merged while backend changes are in flight.Integration with the multichain asset list will occur in a separate PR. We'll likely want to add additional e2e tests when that happens.
Running this branch
yarn && yarn setup
PORTFOLIO_VIEW=1 yarn watch
yarn start:ios
ori
from the watch processPlease ensure that this PR looks okay both with and without the feature flag running. I have introduced a horizontal scroll view because there simply was not enough screen real estate to make the network filter look okay with truncated text.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Screen.Recording.2024-10-29.at.1.25.18.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist