feat: Token Network Filter UI [Extension] #27884
Open
+448
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
gambinish
changed the title
|
Demo of WIP: Screen.Recording.2024-10-17.at.7.43.44.AM.mov |
…g custom filter logic
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
gambinish
added
team-assets
needs-assets-ux-review
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
Builds ready [5b40705]
Page Load Metrics (1979 ± 122 ms)
|
Builds ready [f7f09da]
Page Load Metrics (1994 ± 76 ms)
|
Builds ready [c6a408d]
Page Load Metrics (2046 ± 72 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
andreahaku
reviewed
Oct 24, 2024
| </Text> | ||
| </Box> | ||
| <Box display={Display.Flex}> | ||
| {Object.values(allNetworks).map((network, index) => { |
There was a problem hiding this comment.
what about:
Object.values(allNetworks).slice(0, 5).map(({ chainId }, index) => {
const networkImageUrl = CHAIN_ID_TO_NETWORK_IMAGE_URL_MAP[chainId as keyof typeof CHAIN_ID_TO_NETWORK_IMAGE_URL_MAP];
return (
<AvatarNetwork
key={chainId}
name="All"
src={networkImageUrl ?? undefined}
// overlap the icons
style={{
marginLeft: index === 0 ? 0 : '-20px',
zIndex: 5 - index, // Since we're limiting to 5 networks, zIndex is 5 - index
}}
/>
);
});
There was a problem hiding this comment.
Good callout, addressed here: a2074cd
There was a problem hiding this comment.
And here: 836a11b
| if (filterCallback && opts) { | ||
| switch (filterCallback) { | ||
| case 'inclusive': | ||
| if (typeof nestedValue === 'string') { |
There was a problem hiding this comment.
what if you create helper functions to handle inclusive filter and range filter like:
// Helper function for 'inclusive' filter
function handleInclusiveFilter(nestedValue: unknown, opts: Record<string, boolean>): boolean {
return typeof nestedValue === 'string'
? filterCallbacks.inclusive(nestedValue, opts)
: false;
}
// Helper function for 'range' filter
function handleRangeFilter(nestedValue: unknown, opts: { min: number; max: number }): boolean {
return typeof nestedValue === 'number'
? filterCallbacks.range(nestedValue, opts)
: false;
}
so that you can then have something much more readable like:
switch (filterCallback) {
case 'inclusive':
return handleInclusiveFilter(nestedValue, opts);
case 'range':
return handleRangeFilter(nestedValue, opts);
default:
return true;
}
There was a problem hiding this comment.
Yeah you're right this could be cleaner. Rather than adding the additional abstraction, I ended up just cleaning up the code in the switch statement, would you be fine with this?
switch (filterCallback) {
case 'inclusive':
return filterCallbacks.inclusive(
nestedValue as string,
opts as Record<string, boolean>,
);
case 'range':
return filterCallbacks.range(
nestedValue as number,
opts as { min: number; max: number },
);
default:
return true;
}
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
Builds ready [a2074cd]
Page Load Metrics (2146 ± 104 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
Builds ready [441d4cf]
Page Load Metrics (2190 ± 100 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds Token network filter controls. Note that this is not fully functional, and is currently blocked by two PRs before it can be fully integrated:
In the meantime, this PR is set behind a feature flag
FILTER_TOKENS_TOGGLEand can be run as follows:FILTER_TOKENS_TOGGLE=1 yarn webpack --watchAlternatively:
FILTER_TOKENS_TOGGLE=1 yarn startIncluded in this PR:
tokenNetworkFilterpreference to PreferencesController to manage which chains should be considered when filtering tokens{ [chainId]: true) }this is meant to be flexible enough to support multiple chains in the future.filterAssetsfunction in a similar style tosortAssetsit should be configuration based, and should be extensible enough to support filtering assets by deeply nested values (NFT traits), and to also support complex filter types (like price ranges).Not included in this PR:
Related issues
https://github.com/orgs/MetaMask/projects/85/views/35?pane=issue&itemId=82217837
https://consensyssoftware.atlassian.net/browse/MMASSETS-430
Manual testing steps
Token Filter selection should persist through refresh
Current chain balance should reflect the balance of the current chain
Should visibly match designs: https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=5750-47217&node-type=canvas&t=EjOUPnqy7tWZE6sV-0
Screenshots/Recordings
Screen.Recording.2024-10-22.at.10.46.39.AM.mov
Pre-merge author checklist
Pre-merge reviewer checklist