Adds open, onOpenChange, and anchorRef props to DropdownMenu #1372
Conversation
🦋 Changeset detectedLatest commit: 8f67fe8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
|
Let me know if yall think this merits a test! |
| const renderMenuAnchor = useCallback( | ||
| <T extends React.HTMLAttributes<HTMLElement>>(props: T) => { | ||
| return renderAnchor({ | ||
| const renderMenuAnchor = useMemo(() => { |
There was a problem hiding this comment.
Just curious, what happens if both renderAnchor and anchorRef are passed to DropdownMenu?
There was a problem hiding this comment.
If they're both passed, the renderAnchor function is called with anchorRef as a prop (which would mean putting the same ref in two DOM nodes, which seems wrong but I don't know how it'd break. It's almost certainly not an intended usage, but I'm aiming to mirror what I saw elsewhere.
I spoke with @dgreif about this yesterday, and it sounds like the prop types in AnchoredOverlay could have been narrowed (specifically in AnchoredOverlayPropsWithAnchor such that anchorRef did not exist), except it made destructuring props messier.
| </Box> | ||
| </Box> | ||
| <DropdownMenu | ||
| renderAnchor={null} |
There was a problem hiding this comment.
Is explicitly setting renderAnchor to null required in order to use anchorRef? If so, I can see that being a confusing API for consumers.
There was a problem hiding this comment.
Yep. I tried to mirror SelectPanel, where null and undefined have two distinct uses:
- Passing
undefinedmeans that the component should provide a defaultrenderAnchormethod nullmeans that the component should not provide a default, and instead pass thenullthrough to AnchoredOverlay (in the case where you wanted AnchoredOverlay to just use theanchorReffor anchoring).
It's a little wonky, but my inclination is to try to clean all of these up in a future PR. I'd like to hear your thoughts though!
src/DropdownMenu/DropdownMenu.tsx
Outdated
| @@ -33,35 +29,53 @@ export interface DropdownMenuProps extends Partial<Omit<GroupedListProps, keyof | |||
| * Props to be spread on the internal `Overlay` component. | |||
| */ | |||
| overlayProps?: Partial<OverlayProps> | |||
|
|
|||
| /** | |||
| * If defined, will control the open/closed state of the overlay. Must be used in conjuction with `onOpenChange`. | |||
There was a problem hiding this comment.
Should we be explicit here about how the open state is managed if open is not defined? (i.e. it becomes an uncontrolled component)
Co-authored-by: Cole Bemis <[email protected]>
This extends
DropdownMenuto allow:openandonOpenChangepropsanchorRefprop to control the positioning of the overlay.Closes #1369.
Screenshots
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.