Conversation
mdespuits
requested review from
dkordik,
nvmusoke,
pixelbandito and
sebastianvera
pixelbandito
left a comment
pixelbandito
left a comment
There was a problem hiding this comment.
I only have some suggestions for typing, so I'll go ahead and approve it.
| group?: string; | ||
| /** This will be the array key and the fallback contents */ | ||
| label: string; | ||
| onClick?: () => void; |
There was a problem hiding this comment.
You might be able to omit children and onClick from this definition, since those types should be defined already in the ItemProps (composed from ButtonProps, etc.)
There was a problem hiding this comment.
removed.. tested in storybook and local dev environment and everything works as expected.
| onClick?: () => void; | ||
| }; | ||
|
|
||
| interface EasyDropdownProps extends DropdownProps { |
There was a problem hiding this comment.
Here too, you might be able to omit many of these props, since they're already provided by DropdownProps
At least, these look redundant: children, className, disabled, isOpen
|
|
||
| // Group menu items while preserving their order. | ||
| const menuItemGroups = useMemo( | ||
| const menuItemGroups = useMemo<Record<string, MenuItem[]>>( |
There was a problem hiding this comment.
Super minor: You might not need to define the type here, since TS is sometimes pretty smart about inferring types.
I think it's worth trying, but not a big deal if TS demands you leave it in.
| export type ButtonElementProps = React.ButtonHTMLAttributes< | ||
| HTMLButtonElement | ||
| > & { | ||
| href?: undefined; |
There was a problem hiding this comment.
hmm, I'm coming in without a lot of context here, but why are we creating Button and Div types that have href props?
pixelbandito
left a comment
pixelbandito
left a comment
There was a problem hiding this comment.
One little comment, but I'll pre-approve. Thank you for taking care of this!
| className?: string; | ||
| export type ButtonElementProps = React.ButtonHTMLAttributes<HTMLButtonElement>; | ||
|
|
||
| type AnchorElementProps = React.AnchorHTMLAttributes<HTMLAnchorElement> & { |
There was a problem hiding this comment.
Just curious: Anchor elements should already have href - did it need to be added here?
There was a problem hiding this comment.
😬 It was not needed. I removed it.
4 participants
Fixes #196