[dialog] Fix popup prop merging #2119
Conversation
commit: |
Bundle size reportTotal Size Change: ▼-934B(-0.06%) - Total Gzip Change: ▼-191B(-0.04%) Show details for 41 more bundles@base-ui-components/react/select parsed: ▼-132B(-0.11%) gzip: ▼-15B(-0.04%) |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
atomiks
force-pushed
the
fix/dialog-prop-merging
branch
from
b554758 to
8042a9d
Compare
| @@ -48,7 +48,7 @@ export default function MenuComplexNestingExperiment() { | |||
|
|
|||
| <Dialog.Root> | |||
| <Menu.Item | |||
| render={<Dialog.Trigger />} | |||
| render={<Dialog.Trigger nativeButton={false} />} | |||
There was a problem hiding this comment.
This doesn't seem correct. Dialog.Trigger renders a native button. If there was <Dialog.Trigger render={<Menu.Item />}> we'd need nativeButton={false}.
There was a problem hiding this comment.
Actually, it should be <Menu.Item render={<Dialog.Trigger />} nativeButton />, no?
There was a problem hiding this comment.
Yes, right - imo this area needs work in the API as it's pretty confusing and an easy footgun
There was a problem hiding this comment.
It could be alleviated by us using native buttons everywhere, including menu items. Then, only if devs want to use noninteractive elements, they'll have to explicitly add the prop.
With #2042, merging with main I noticed two issues:
nativeButton={false}must be specified onDialog.Triggersince theMenu.Itemis rendering it, which is not a native button (the keyboard handlers didn't work, only the pointer). I wonder if this can be made more obvious/clear.useDialogPopup, causing theonKeyDownhandler to overwrite and break the handlers that handled theEscapeevent key bubbling