-
Notifications
You must be signed in to change notification settings - Fork 635
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
Convert Chain Selection DropdownMenu to Backend Networks #6344
Convert Chain Selection DropdownMenu to Backend Networks #6344
Conversation
src/components/DropdownMenu.tsx
Outdated
@@ -75,6 +92,8 @@ export function DropdownMenu<T extends string>({ | |||
side = 'right', | |||
alignOffset = 5, | |||
avoidCollisions = true, | |||
hitSlop = 12, // TODO: Figure out how to make the trigger hit slop work |
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.
note for self
}, | ||
[convertAssetAndNavigate] | ||
); | ||
const handlePressContextMenu = useCallback((chainId: string) => convertAssetAndNavigate(+chainId), [convertAssetAndNavigate]); |
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.
maybe (not sure if good idea) we could make convertAssetAndNavigate
take string and convert inside, to not need the useCallback
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.
ah actually just notice DropdownMenu
is not memo
ed so the fn being stable or not does not make a difference, the useCallback is just overhead rn, don't think we can easily memo it since it takes children
}, | ||
})); | ||
|
||
const MenuWrapper = availableChainIds.length > 1 ? ContextMenuButton : Box; | ||
const Children = useMemo(() => { |
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.
children is memo maybe it works I never tested returning components from useMemo
const backendNetworks = get().backendNetworks; | ||
return backendNetworks.networks.reduce( | ||
(acc, backendNetwork) => { | ||
acc[parseInt(backendNetwork.id, 10)] = backendNetwork.icons.badgeURL; |
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.
I see we do this parseInt(x, 10)
sporadically why is that? what's the second arg 10 for?
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.
you can call getChainsBadgeWorklet
here
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.
That's true. I will probably do a cleanup PR to this file here soon but don't want to muddy it in this PR.
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 changed (plus any additional context for devs)
Changes all context menus that are chain selectors to use Zeego. We now no longer rely on local ios/ assets to deliver the chain badges inside of those dropdowns and can rely on remote backend networks. These are located in three places
Work that's left to do
Screen recordings / screenshots
https://rainbowhaus.slack.com/archives/C0468CDBE75/p1734480680880539
What to test
need to test that functionally the chain selection in all these places remains unchanged.