-
-
Notifications
You must be signed in to change notification settings - Fork 267
[menu] Support detached triggers #3170
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
Conversation
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
9b462ac to
916dbd9
Compare
commit: |
50d12bd to
4ff5a85
Compare
4ff5a85 to
da41ab9
Compare
|
This is also on main, but not in beta.4, perhaps it should be fixed here though: On https://deploy-preview-3170--base-ui.netlify.app/experiments/popups/popups-in-popups, click-to-drag on the menu to a submenu item and releasing closes the dialog unexpectedly |
|
On https://deploy-preview-3170--base-ui.netlify.app/experiments/menu/nested-detached-triggers,
|
|
|
|
||
| return React.useMemo(() => (enabled ? { reference } : {}), [enabled, reference]); | ||
| return React.useMemo( | ||
| () => (enabled ? { reference, trigger: reference } : {}), |
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's the value of adding the trigger as key, if it's always the same as the reference value?
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.
useInteractions returns methods like getReferenceProps and getTriggerProps that use these keys. MenuTrigger calls these methods depending on whether it's an active trigger. Previously, nothing would be returned in the getTriggerProps call.
| ); | ||
| } | ||
|
|
||
| function ReusableMenu(props: { handle: Menu.Handle<ContentKey> }) { |
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 really like the API, it's very clean 👌
|
In the Menubar experiment, the safe polygon logic broke on the 'Add to playlist' submenu |
|
Another bug I noticed in the Menubar is that if I open a submenu, the other menus no longer open on hover. There is also a flicker if I try to reopen the same submenu, see the recording attached. Screen.Recording.2025-11-13.at.09.11.18.mov |
303c29f to
7969c67
Compare
bc0b301 to
e34b1bb
Compare
atomiks
left a comment
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.
Haven't been able to find regressions in regular menus - in the docs, some of the components need to be updated to use the tag format <Menu.Root> (not Menu.Root)
Implemented detached / multiple trigger support in Menu, similarly to #2336
It is now possible to define triggers outside of Menu.Root using the handle prop. Multiple triggers per menu with a dynamic payload are also implemented.
Detached triggers also work in Menubar menus.
Content transitions will be implemented in a follow-up PR.
Closes #3031
Preview
Breaking changes
openOnHover,delay, andcloseDelayprops moved from Menu.Root to Menu.Trigger.