-
Notifications
You must be signed in to change notification settings - Fork 5k
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: Filters UI (desktop & mobile) #12299
Conversation
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.
On the functional testing side, I think the only thing I would question is the language selector. It doesn’t work in the same way as the other filters. When you select a language:
- I would expect to see the selected language displayed instead of the “Search language” placeholder
- The filter is working fine but it is not shown in the “Filters (0)” counter
These issues can cause confusion for the user when trying to understand what filters are being applied.
I see a visual bug in mobile. The modal footer sometimes gets behind the content
=======
On the code side, I've left a couple of suggestions/questions, nothing blocking. Looks good overall.
Great job @nhsz 💯
> | ||
<Box mx={{ base: 0, md: 0, lg: 2 }} w="100%"> | ||
{/* Category label */} | ||
<Heading as="h4" lineHeight={1.4} fontSize="md" fontWeight="bold" mb={2}> |
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.
<Heading as="h4" lineHeight={1.4} fontSize="md" fontWeight="bold" mb={2}> | |
<Heading as="h4" size="md" mb={2}> |
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.
size="md"
is not the same font size defined in the design @konopkja
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.
Small note, if I select a filter, then select a persona, the filter toggle stays on, but the checkboxes get unchecked.
Screen.Recording.2024-03-19.at.09.32.27.mov
Otherwise this is looking great.. functionally it's working as expected aside from the couple things noted. Just left some clean up notes, and a few things we could discuss to address separately (ie using standard sizes for a handful of things)
Nice job @nhsz! Excited to move this forward
key={feature.label} | ||
spacing="0.2rem" | ||
fontSize="0.9rem" |
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.
Again not a blocker, but cc: @nloureiro if there is a way we can try to work our way towards standard font sizes
outline="1.5px solid" | ||
outlineColor="primary.base" | ||
outlineOffset="3px" | ||
fontSize={8} |
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.
fontSize={8} is 2rem correct? @nloureiro Do you know how often we're trying to use this font size? Not blocking for this, but again noting we have some standard font sizes that would be nice to be able to stick to:

…-website into filters-ui-desktop
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.
Awesome @nhsz, lgtm! gj
The only thing left for me (which I don't see as a blocker) is this issue on mobile:
I keep seeing this on a physical phone when the top frame of the browser (where the url input is) is shown.
…-website into filters-ui-desktop
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.
Looks good! Thanks @nhsz! Just left one comment about the hideFrom
syntax, but we don't need to deal with that here, just noting. Pulling in when build finishes! 🎉
[WIP] This PR implements the UI updates and changes for the wallet filters
Preview: https://deploy-preview-12299--ethereumorg.netlify.app/en/wallets/find-wallet/
Description
WalletFilterProfile
componentWalletFilterSidebar
componentWalletsTable
stylesbackground.highlight
when expandedWalletMoreInfo
stylesWalletMoreInfoCategory
stylesWalletFilterPersona
componentSupportedLanguagesTooltip
componentMobileFiltersMenu
component (see new mobile menu design)ResetFiltersButton
componentLanguageSupportFilter
componentMobileFiltersButton
componentfeatureDropdownItems
filterOptions
Advanced
label/category to filterseip_1559_support
feature filtergetPersonaBorderColor
utilgetLocaleFormattedDate
utilgetLanguageCodeName
utilcapitalize
utilgetAllWalletsLanguages
utilwalletsListingCount
utilgetWalletsTopLanguages
utiluseWalletTable
inside/hooks
diruseWalletFilterFeature
inside/hooks
dirWALLETS_FILTERS_DEFAULT
constantWalletFilter
typeWalletSupportedLanguageContextType
typeEIP1559Icon
wallet.last_updated
formatpage-find-wallet-personas-title
keypage-find-wallet-see-wallets
keypage-find-wallet-non-custodial
valuepage-find-wallet-connect-to-dapps-desc
valuepage-find-choose-to-compare
unused keypage-find-wallet-choose-to-compare
unused keypage-find-wallet-persona-desc
unused keypage-find-wallet-profile-filters
unused keypage-find-wallet-feature-filters
unused keypage-find-wallet-fee-optimization
unused keypage-find-wallet-fee-optimization-desc
unused keypage-find-wallet-mobile-desc
unused keypage-find-wallet-desktop-desc
unused keypage-find-wallet-browser-desc
unused keypage-find-wallet-hardware-desc
unused keypage-find-wallet-choose-features
unused keypage-find-wallet-walletconnect
unused keypage-find-wallet-walletconnect-desc
unused keypage-find-wallet-token-support
unused key