feat: DropdownMenu #1704
feat: DropdownMenu #1704
Conversation
|
Visit the preview URL for this PR (updated for commit 656479e): https://penumbra-ui-preview--pr1704-feat-prax-160-menu-mvj0lz31.web.app (expires Mon, 26 Aug 2024 08:54:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
packages/ui/src/Button/index.tsx
Outdated
| @@ -200,6 +201,7 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>( | |||
| ? theme.borderRadius.sm | |||
| : theme.borderRadius.full | |||
| } | |||
| {...props} | |||
There was a problem hiding this comment.
In Radix, asChild prop will only work if the nested component passes the rest props.
It doesn't mean we allow users to pass whatever they want - at least TypeScript disagrees with it
There was a problem hiding this comment.
Ah! Great solution. I was kinda bothered by the fact that I wasn't previously spreading remaining props when Radix wants us to, but I didn't want to change it for TypeScript reasons.
request: could you add a note documenting this? so that future maintainers don't think that there should be add'l props in TypeScript?
| * | ||
| * @see https://www.radix-ui.com/primitives/docs/guides/composition | ||
| */ | ||
| asChild?: boolean; |
There was a problem hiding this comment.
Removed asChild prop from the Popover.Trigger and DropdownMenu.Trigger. Since we do not users to change the component styles, it makes to sense to allow asChild={false} – it will always be an unstyled HTML-element
There was a problem hiding this comment.
OK. I could see there being a use case where users want to just have plain text as the trigger. But maybe that's not even a good idea. So, no strong opinion here.
| DropdownMenu.RadioItem = RadioItem; | ||
| DropdownMenu.CheckboxItem = CheckboxItem; | ||
| DropdownMenu.Item = Item; |
There was a problem hiding this comment.
These components represent the MenuItem from the design
| &:focus:not(:disabled) { | ||
| background-color: ${props => props.theme.color.action.hoverOverlay}; | ||
| outline: 2px solid ${props => getOutlineColorByActionType(props.theme, props.$actionType)}; | ||
| } |
There was a problem hiding this comment.
the styles do not have :hover and we can't change it – Radix does not distinguish between hover and focus for the menu items. It means that hovering an item also makes it focused to not confuse the states: radix-ui/primitives#956 (comment)
| * | ||
| * @see https://www.radix-ui.com/primitives/docs/guides/composition | ||
| */ | ||
| asChild?: boolean; |
There was a problem hiding this comment.
OK. I could see there being a use case where users want to just have plain text as the trigger. But maybe that's not even a good idea. So, no strong opinion here.
| onChange?: (value: boolean) => void; | ||
| } | ||
|
|
||
| export const CheckboxItem = ({ |
There was a problem hiding this comment.
suggestion: keep the dropdown menu open when selecting checkbox items
I couldn't figure out a way to do this in the docs, nor do I see in the code where you're closing the dropdown menu once a user clicks a menu item (maybe Radix does it automatically?). But it'd be nice to keep the dropdown menu open when clicking checkbox items specifically, since the user may wish to click more than one.
Not a blocker for this PR, though !
There was a problem hiding this comment.
We should probably investigate the use cases with time. Leaving as-is for now
one last PR review for the books 📚 |
Closes prax-wallet/prax#160
Implement not exactly
MenuItembutDropdownMenubecause theMenuItemfrom Figma designs is meant to be used within the dropdown. Radix-ui has great support for these component with a11y enhancement.The
MenuItemfrom Figma is used forDropdownMenu.Item,DropdownMenu.RadioItem, andDropdownMenu.CheckboxItemcomponents.Design – https://www.figma.com/design/WKgXSVVK3L0EI0sr9Hwjoa/penumbra-UI-library?node-id=778-49265&m=dev
Storybook – https://penumbra-ui-preview--pr1704-feat-prax-160-menu-mvj0lz31.web.app/?path=/docs/ui-library-dropdownmenu--docs