feat: new token selection modal #711
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| )} | ||
|
|
||
| {error ? ( | ||
| <Alert variant="filled" severity="error"> |
There was a problem hiding this comment.
Adding an invalid token in the pool creation page results in this
Tried with 0x1D774ed0A7b897aAaE3526F07e487C5F9540F55D
There was a problem hiding this comment.
The popup gets stuck afterwards, even closing and reopening the modal doesn't clear the error and adding a good token doesn't work anymore
There was a problem hiding this comment.
I've made the error message grow in height as needed. The height is actually fixed to IconSize.sm in defineMuiAlertTitle. I didn't really want to introduce an entire variant for this one off, so I've opted for sx={{ '&': { height: 'auto' } }} for now.
The error will also go away when you close the modal or change the search value.
Not sure what's the best way to tack on a friendly error message. I could change any error to "invalid token" but maybe it's not related to the token but the RPC. Or maybe there's something else, and then you'd want to know the full error. Not sure we can just look at the error message and search for "Multicall3", replace it with "Invalid token" and call it a day
| import { FavoriteTokens } from './FavoriteTokens' | ||
| import { TokenOption } from './TokenOption' | ||
|
|
||
| // Prevent all the token options from re-rendering if only the balance of a single one has changed. |
There was a problem hiding this comment.
This shouldn't be necessary, it should happen automatically 🤔
There was a problem hiding this comment.
I tried with and without and didn't notice / measure any difference, so I've removed it.
There was a problem hiding this comment.
I really need to give virtualization a second try though, when you first open the modal with all tokens the performance is dogshit imo. Not the case when I limit the list to just 5 items. It's also not the re-renders causing the performance hog, checked it with a console.log(symbol) inside TokenOption.tsx.
| }), | ||
| }} | ||
| > | ||
| {/* I'm abusing the symbol here to deliberately show a tooltip for the address instead */} |
There was a problem hiding this comment.
I've experimented with showing the address on hover and keyboard focus at the place where the label would be, but because we're not using labels it causes the token symbol to move up. See video attached, the symbol typography has to move up to make room for the address to show up. When no longer hovering or focusing the item the address would go away and the symbol would move down again.
This looks very jumpy and distracting so I've opted to keep the token on hover for now
Video.mp4
apps/main/src/dex/components/PageCreatePool/TokensInPool/SelectTokenButton.tsx
Outdated
Show resolved
Hide resolved
| ['desc'], | ||
| ).map(({ address }) => address) | ||
| const selectToList = Object.entries(tokensMapper) | ||
| .map(([_, v]) => v!) |
There was a problem hiding this comment.
This looks incorrect - if tokensMapper has no null/undefined why is it typed so?
There was a problem hiding this comment.
Probably because export type TokensMapper = { [tokenAddress: string]: Token | undefined }
If I remove the exclamation mark I actually get a type error that the address in .map(({ address }) => address) might be undefined as the type is Token | undefined. It's a bit silly because then I don't even know if you can have a key whose value is undefined?
packages/curve-ui-kit/src/features/select-token/ui/TokenSelectButton.tsx
Outdated
Show resolved
Hide resolved
packages/curve-ui-kit/src/features/user-profile/settings/hide-small-pools/HideSmallPools.tsx
Outdated
Show resolved
Hide resolved
| styleOverrides: { | ||
| root: { | ||
| '&:focus': { | ||
| outline: 'none', |
There was a problem hiding this comment.
Are you sure we want to disable this? It might be useful for accessibility. How about focus-visible?
For buttons and icon buttons I use '&:focus-visible': { borderColor: Focus_Outline } as discussed with UX.
There was a problem hiding this comment.
The menu list has an auto-focus so you can use keyboard navigation immediately to navigate the list. However, that means it immediately renders with a damn awful black border. I could change &:focus to &:focus-visible, but showing any kind of outline is not an option, so neither would be using Focus_Outline
Various:
Main:
Highlights:
I've refrained from refactoring old code as much as possible, which means that the initial fetching of the token list is still slow as hell. I don't want to inflate the PR with even more code changes than it already has.