-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
[Toolbar] Add Toolbar components #1349
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f2b6d91
to
9c3111f
Compare
2901a4d
to
61dc192
Compare
@@ -22,6 +23,8 @@ export function useButton(parameters: useButton.Parameters = {}): useButton.Retu | |||
rootElementName: elementNameProp, | |||
}); | |||
|
|||
const isCompositeItem = useCompositeRootContext(true) !== undefined; |
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.
This is to work around using Select.Trigger
as a composite item by letting CompositeItem
's tabIndex override the internal one in useButton
Like Menu.Trigger
, select trigger manually sets tabIndex to keep a consistent "trigger remains focused after a select/menu item is select" behavior across browsers (Safari on iPadOS and iOS): https://github.com/mui/base-ui/blob/master/packages/react/src/select/trigger/useSelectTrigger.ts#L80
9dd6644
to
c28fe0d
Compare
c329fef
to
5dcde0c
Compare
5dcde0c
to
5fe85d2
Compare
<Toolbar.Button | ||
render={<Toggle />} |
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.
<Toolbar.Button
render={
<Toggle
pressed={dialogOpen}
onPressedChange={(nextPressed) => {
setDialogOpen(nextPressed);
}}
>
<Icon />
</Toggle>
}
/>
If I tried to make this a controlled toggle, this part of mergeReactProps
errors when it tries to merge our custom handlers, as the event arg is not the first: https://github.com/mui/base-ui/blob/master/packages/react/src/utils/mergeReactProps.ts#L44
Uncaught TypeError: Cannot create property 'preventBaseUIHandler' on boolean 'true'
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 found a strange behavior: the menu won't close when a submenu is open and you move the roving focus to the next toolbar element by pressing ➡️. Only the submenu is closed. See attached video. Kapture.2025-02-07.at.07.45.58.mp4 |
Hmm this is tricky, it probably has to be handled internally in Menu somehow. I think there could be a big overlap with |
{toolbarContext ? ( | ||
renderElement() | ||
) : ( | ||
<CompositeRoot direction={direction} loop={loop} render={renderElement()} /> | ||
)} |
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.
Is it a bit sketchy to need a component to have to check another somewhat-arbitrary component's context?
Demos:
TODO
Component integrations
Closes #661