-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 [Extension] #27884
base: develop
Are you sure you want to change the base?
Conversation
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. |
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
…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!]
|
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.
few code optimisation and readability comments :)
</Text> | ||
</Box> | ||
<Box display={Display.Flex}> | ||
{Object.values(allNetworks).map((network, index) => { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout, addressed here: a2074cd
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.
And here: 836a11b
if (filterCallback && opts) { | ||
switch (filterCallback) { | ||
case 'inclusive': | ||
if (typeof nestedValue === 'string') { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!]
|
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_TOGGLE
and can be run as follows:FILTER_TOKENS_TOGGLE=1 yarn webpack --watch
Alternatively:
FILTER_TOKENS_TOGGLE=1 yarn start
Included in this PR:
tokenNetworkFilter
preference 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.filterAssets
function in a similar style tosortAssets
it 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