-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
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
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 <[email protected]>
Docs:
https://deploy-preview-1349--base-ui.netlify.app/react/components/toolbar
Demos:
Summary
Root
,Button
,Input
,Link
,Group
,Separator
focusableWhenDisabled
prop onButton
&Input
orientation
proploop
prophotkey prop on Root[toolbar] Support an optional hotkey #1454cols
propComponent integrations
The three demos linked above show all of these:
Toolbar.Input
Inputthis will be aToolbar.Input
insteadCloses #661