-
-
Notifications
You must be signed in to change notification settings - Fork 263
[Toolbar] Add Toolbar components #1349
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
✅ 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
| 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
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 wonder if this could cause problems with buttons nested inside other components within a toolbar (for example, a button inside a dialog opened from a toolbar would have this set to true, even though it's not a composite item).
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.
for example, a button inside a dialog opened from a toolbar
Only if that button happened to use useButton as well, so in this case, a disabled Dialog.Close isn't affected, but a MenuTrigger in that dialog would be
What this affects is that when disabled, the HTML disabled attribute is never applied. Generally I think our components that use useButton already don't rely on the disabled attr to disable functionality anyway in order to support component/element replacement so it should be OK in most cases
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.
@michaldudak I also tried making it a isCompositeItem param instead of a check but it wasn't enough to handle this more common case: 575dc96
5dcde0c to
5fe85d2
Compare
|
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 |
This comment was marked as outdated.
This comment was marked as outdated.
090aee3 to
b3d1c0c
Compare
c865bad to
626f251
Compare
736a277 to
7b40b0c
Compare
b13ec09 to
86eaed3
Compare
|
I've made the demo smaller and removed the icons/input @colmtuite |
|
Content
Styling
|
|
@colmtuite Updated the demo and docs: https://deploy-preview-1349--base-ui.netlify.app/react/components/toolbar If this is ok I'll add the tailwind version as well |
Co-authored-by: atomiks <cc.glows@gmail.com>


Docs:
https://deploy-preview-1349--base-ui.netlify.app/react/components/toolbar
Demos:
Summary
Root,Button,Input,Link,Group,SeparatorfocusableWhenDisabledprop onButton&Inputorientationproploopprophotkey prop on Root[toolbar] Support an optional hotkey #1454colspropComponent integrations
The three demos linked above show all of these:
Toolbar.InputInputthis will be aToolbar.InputinsteadCloses #661