feat(Toolbar): add menu prop #1518
Conversation
miroslavstastny
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1518 +/- ##
=========================================
- Coverage 71.82% 71.6% -0.23%
=========================================
Files 829 835 +6
Lines 6697 6789 +92
Branches 1917 1927 +10
=========================================
+ Hits 4810 4861 +51
- Misses 1882 1922 +40
- Partials 5 6 +1
Continue to review full report at Codecov.
|
| * @param {SyntheticEvent} event - React's original SyntheticEvent. | ||
| * @param {object} data - All props and proposed value. | ||
| */ | ||
| onMenuOpenChange?: ComponentEventHandler<ToolbarItemProps> |
There was a problem hiding this comment.
Should we also hoist onOpenChange from Popup to here to have both on the same level?
There was a problem hiding this comment.
Let's skip it for now, we still have an access to it via popup shorthand
miroslavstastny
added
🚀 ready for review
🚧 WIP
and removed
🚧 WIP
🚀 ready for review
labels
docs/src/examples/components/Toolbar/Types/ToolbarExampleEditor.shorthand.tsx
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Toolbar/Types/ToolbarExampleEditor.shorthand.tsx
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Toolbar/Types/ToolbarExampleEditor.shorthand.tsx
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Toolbar/Types/ToolbarExampleEditor.shorthand.tsx
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Toolbar/Types/ToolbarExampleMenu.shorthand.tsx
Outdated
Show resolved
Hide resolved
| <Ref innerRef={this.menuRef}> | ||
| <Popper align="start" position="above" targetRef={this.itemRef}> | ||
| {ToolbarMenu.create(menu, { | ||
| overrideProps: predefinedProps => ({ |
There was a problem hiding this comment.
| overrideProps: predefinedProps => ({ | |
| overrideProps: (predefinedProps: ToolbarItemProps) => ({ |
There was a problem hiding this comment.
Are we going to move overrideProps to a class method?
There was a problem hiding this comment.
I don't think so. This is the only place where it is called, just once.
| </ElementType> | ||
| ) | ||
|
|
||
| if (!wrapper) { |
There was a problem hiding this comment.
| if (!wrapper) { | |
| if (_.isNil(wrapper)) { |
Otherwise it will fail on empty string or 0
There was a problem hiding this comment.
This is intentional. wrapper: false is valid.
There was a problem hiding this comment.
_.isNil(wrapper) || wrapper === false?
packages/react/src/themes/teams/components/Toolbar/toolbarMenuDividerVariables.ts
Show resolved
Hide resolved
| @@ -135,6 +161,31 @@ class ToolbarItem extends UIComponent<WithAsProp<ToolbarItemProps>, ToolbarItemS | |||
| </ElementType> | |||
| ) | |||
|
|
|||
| const submenu = menuOpen ? ( | |||
There was a problem hiding this comment.
what if the menu's items is an empty array?
There was a problem hiding this comment.
Then in renders empty submenu, consistent with Menu. Do you think it is incorrect?
There was a problem hiding this comment.
I would say if there are no menu items we don't need to render it. What the use case of rendering an empty menu?
There was a problem hiding this comment.
Agree, but I think it is consumer's responsibility. menu can be array or object or element and we cannot check all. So I think it is better to be consistently dump than to try to be smart but inconsistent.
| <> | ||
| <Ref innerRef={this.itemRef}>{renderedItem}</Ref> | ||
| {submenu} | ||
| </> |
There was a problem hiding this comment.
based on our discussion and ARIA spec, toolbar button and menu should be wrapped in one div so then a screen reader will work correctly. @jurokapsiar correct me if I am wrong
Another aspect, when the focus is in menu and press ESC, the menu should get closed and focus should go on the button.
In theory, this type of keyboard event should be caught by a wrapper element, which wraps a toolbar button and menu and put a focus on the button. That seems for me the most logical way of doing it. Let me know what you think.
There was a problem hiding this comment.
Makes sense. I added a wrapper slot, it is rendered only if there is a menu prop defined on the item, no matter whether the menu is open ( = rendered) or closed ( = not in dom).
Closes #1411.
A Toolbar item can now have a menu:
API
Following props were added to the
ToolbarItem:menu- depending onkindeither shorthand forToolbarMenuItemorToolbarMenuDivider, each Toolbar Menu Item:contentandiconactiveanddisabledonClick,onFocusandonBlurcallbackmenuOpen- boolean, the menu is rendered iftrue.onMenuOpenChange- callback to requestmenuOpenvalue change (the wholeToolbaris controlled component, parent must handle its state).Known Limitations
accessibility to be handled separately
no styles for active (no redlines)
no submenu support (out of scope)
changelog