-
Notifications
You must be signed in to change notification settings - Fork 54
feat(Toolbar): add support for radio & checkbox groups #1920
Conversation
…at/toolbar-groups # Conflicts: # CHANGELOG.md
return ToolbarMenuDivider.create(item, { overrideProps: dividerOverridesFn }) | ||
|
||
switch (kind) { | ||
case 'checkbox': |
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.
just noticed that we call it toggle
in the Toolbar
component. We should probably have the same naming across these two
…at/toolbar-groups # Conflicts: # CHANGELOG.md # packages/react/src/themes/teams/components/Toolbar/toolbarMenuItemStyles.ts
Codecov Report
@@ Coverage Diff @@
## master #1920 +/- ##
==========================================
- Coverage 70.52% 70.42% -0.11%
==========================================
Files 884 889 +5
Lines 7810 7841 +31
Branches 2257 2263 +6
==========================================
+ Hits 5508 5522 +14
- Misses 2291 2308 +17
Partials 11 11
Continue to review full report at Codecov.
|
packages/react/test/specs/components/Toolbar/ToolbarMenuRadioGroup-test.ts
Show resolved
Hide resolved
'aria-checked': props.checked, | ||
'aria-disabled': props.disabled, | ||
role: 'menuitemradio', | ||
tabIndex: 0, |
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.
we should not need tabIndex if data-is-focusable is used and the item is in focus zone
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.
Good catch, removed 👍
accessibility?: Accessibility | ||
|
||
/** Index of the currently active item. */ | ||
activeIndex?: number |
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.
to meke it predictable, I think we should add activeIndex to toolbarRadioGroup as well
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.
Added 👍
…react into feat/toolbar-groups
…at/toolbar-groups
@@ -35,12 +35,21 @@ export interface ToolbarMenuItemProps | |||
/** A toolbar item can be active. */ | |||
active?: boolean | |||
|
|||
/** Item can show check indicator if selected. */ | |||
checked?: boolean |
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.
sorry that I missed it before - for toggle in the toolbar we are using toggleButtonBehavior, there the prop is called active
. That way it would even be consistent with the toolbar menu radio.
…om/stardust-ui/react into feat/toolbar-groups
Changes addressed :rocket
This PR:
toggle
kind
of items inToolbarItem
'smenu
group
kind
of items inToolbarItem
'smenu
, which is implemented byToolbarMenuRadioGroup
Based on: https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-2/menubar-2.html