Open diegohaz opened 3 years ago
This makes the renderToggle and renderContent props obsolete as we wouldn't need them to access the internal state anymore and the content could just be passed as children.
🤩
The ToolbarItem
implementation is very cool, I'm glad that it's already written in a generic way like this.
As far as MenuItem
goes, we do have the ItemGroup
and Item
components that look very similar. They can even be rendered inside the existing dropdown: https://wordpress.github.io/gutenberg/?path=/story/components-experimental-itemgroup--dropdown
I wonder if there's some cross over between those components?
Thanks for opening this issue, I like that we're thinking about these component holistically. Would you mind adding some examples about DropdownMenu
does this component remain as, is it deprecated, how does it combine to the other ones. I also see ToolbarMenu
in the examples above, I guess this is the same as the existing ToolbarDropdownMenu
?
As far as
MenuItem
goes, we do have theItemGroup
andItem
components that look very similar. They can even be rendered inside the existing dropdown: https://wordpress.github.io/gutenberg/?path=/story/components-experimental-itemgroup--dropdownI wonder if there's some cross over between those components?
I think it's similar to Item
, but Item
is more about styles than behavior. We could have a generic component for roving tabindex components (like CompositeItem
), but Item
is even more generic than that.
I've also researched about having a generic Item
component that could be used directly for all collection components. So, instead of MenuItem
, ToolbarItem
etc., we could just use Item
:
<Toolbar>
<Item />
<Item />
<Item />
</Toolbar>
<Menu>
<Item />
<Item />
<Item />
</Menu>
<Select>
<Item />
<Item />
<Item />
</Select>
The best example I could find on that was the React Spectrum collection components, but there's a lot of limitations on this approach and I wouldn't really recommend that.
If we want to inject specific behaviors from the parent components into the Item
component, we wouldn't be able to hook into the Item
lifecycle using React hooks. That is, we wouldn't be able to pass useEffect
s down to each Item
. We could technically do that with some custom props, but that would make the whole implementation too complicated, and I don't think it's worth it.
React Spectrum takes a different approach though. Item
is a component that just returns null
. It's used only so they can map through the children and register them. It's a really custom approach that I think may conflict with the React reconciler at some point.
It also doesn't allow us to change the structure of the elements inside a collection. You can only render Item
or Section
inside a collection component (which I guess is inspired by the option
and optgroup
HTML elements). One of the reasons we use custom Select
components is so we can avoid that kind of limitation.
Thanks for opening this issue, I like that we're thinking about these component holistically. Would you mind adding some examples about
DropdownMenu
does this component remain as, is it deprecated, how does it combine to the other ones. I also seeToolbarMenu
in the examples above, I guess this is the same as the existingToolbarDropdownMenu
?
@youknowriad In the examples, Menu
is the same as DropdownMenu
. I'm suggesting a name change so, besides matching the role="menu"
attribute, we also have consistent namespaces across the library. Since we already have the MenuItem
components, I thought it would make more sense to adapt DropdownMenu
than replacing MenuItem
by DropdownMenuItem
.
But I don't really have strong opinions on this. I also understand it may not be really obvious that Menu
is a dropdown as this name is often used to refer to static navigation menus.
But I've seen a lot of people confused about what component they should use, for example, in a Toolbar
. One of the solutions is prefixing all components with Toolbar
(like how we did with ToolbarDropdownMenu
). If we keep DropdownMenu
, I would say we should have DropdownMenuItem
, DropdownMenuItemChoice
etc.
When combining two different modules such as Toolbar
and Menu
, the children of ToolbarMenu
would use the namespace of the last module (Menu
).
So we would have a pattern like this:
<ModuleA>
<ModuleAChild />
<ModuleAChild />
<ModuleAModuleB>
<ModuleBChild />
<ModuleBChild />
</ModuleAModuleB>
</ModuleA>
Do you have some specific use cases in mind so we can add more examples?
It seems like you're suggesting that a Menu is always within a dropdown and that having <Menu><MenuItem><MenuItem></Menu>
where Menu
is not inside a dropdown is not a valid use-case?
In Gutenberg we do have NavigableMenu
which is used in a dozen of places, most of these seem to be Dropdowns or Toolbars but it's not something I can guarantee.
I think I'm ok with not having a non-dropdown Menu
component but I do prefer DropdownMenu
to avoid confusion (like I just got confused). I'm not opinionated about MenuItem
vs DropdownMenuItem
though, I feel MenuItem
is fine and we can also have MenuGroup
(not sure how that fits here) to separate groups of menu items.
It seems like you're suggesting that a Menu is always within a dropdown and that having
<Menu><MenuItem><MenuItem></Menu>
whereMenu
is not inside a dropdown is not a valid use-case?
Yes! If we want a more generic menu (like a navigation menu that's always visible), we would use components like ItemGroup
/Item
or something similar. Those items should be tabbable (and not use roving tabindex as menu items are supposed to) and the container shouldn't use role="menu"
as it could confuse screen reader users.
I think I'm ok with not having a non-dropdown
Menu
component but I do preferDropdownMenu
to avoid confusion (like I just got confused). I'm not opinionated aboutMenuItem
vsDropdownMenuItem
though, I feelMenuItem
is fine and we can also haveMenuGroup
(not sure how that fits here) to separate groups of menu items.
We can keep using MenuGroup
:
<Menu label="Label">
<MenuGroup label="Label">
<MenuItem label="Label" />
<MenuItem label="Label" />
</MenuGroup>
<MenuGroup label="Label">
<MenuItem label="Label" />
<MenuItem label="Label" />
</MenuGroup>
</Menu>
Excellent writeup, @diegohaz. Agreed consistency is a crucial aspect of the library ergonomics since it provides a level of anticipation: if I learn one set of component APIs I can apply the same intrinsic patterns to a new set and expect it to work in similar ways. This allows for discovery and more joyful use of the components when they seem to just work.
For composition purposes this a must and the example of using MenuItem
s instead of an array in props highlights this well. It's reasonable, if I were to be using these APIs, to try adding a MenuItem
inside a Menu
without having to read the documentation for Menu extensively. On the other side, I wouldn't know controls
exist and how it works without reading the docs. Anything else that I might want to attempt, like having groups inside a menu would be more awkward with props.
I personally like the rename to just Menu
, but also don't have a super strong opinion. I think having Dropdown
and DropdownMenu
is more confusing, though. In a way, DropdownMenu
is like a combination of the generic Dropdown
(or Flyout
) and a Menu
. I do prefer MenuItem
for the children in either case.
I'd say the most important thing in API design is having consistent interfaces across different modules, especially when they're related and can be combined together. This lowers the learning curve, making it easier to use other modules when you're already familiar with one of them.
Unfortunately, a solid set of patterns that works consistently across all components in a component library is something really difficult to achieve. I've experienced this with Reakit, but it took a few years before we were able to come up with a set of patterns that would work for all modules in the library, and it's still not completely done. I started writing about this in 2018 on Introducing the Single Element Pattern and then gave a more technical update on that last year on this talk at React Finland.
And this is even more challenging on WordPress components given the components are designed to be more higher-level, which I agree is the right approach here. But it's easier to design component APIs when components render single HTML elements and you don't have to think about passing props to internal elements and customizing how they're rendered, not to mention React Native.
Because of that, instead of trying to include all components, I think we should take those that are related and can be combined together and make their APIs more similar.
Dropdown
(orFlyout
)Currently, we have a Popover component that works as a generic element that can be positioned next to an anchor element or DOMRect. The anchor element can be passed via the
anchorRef
prop. The DOMRect can be passed via theanchorRect
orgetAnchorRect
props.We also have the Dropdown component, which uses the generic
Popover
underneath, but also renders a toggle button via therenderToggle
prop.Finally, we're experimenting with a Flyout component that looks similar to
Popover
andDropdown
, but with a completely different API. The goal here is to find a concise API similar to theDropdown
component API (with very few deprecations), but, ideally, we would use theFlyout
implementation.Aside from custom
Dropdown
props, all the props passed to theDropdown
component, includinglabel
andicon
, would be passed over to the internal toggle button, which would render aButton
by default. This makes thetoggleProps
prop obsolete.The snippet above would be rendered to the DOM as something like this (simplified):
Controlled components
Currently, we have different ways to control the state of the
Popover
components. The genericPopover
component has anonClose
prop.Dropdown
hasonClose
andonToggle
.Dropdown
also accepts render props likerenderToggle
andrenderContent
that passisOpen
,onClose
andonToggle
as arguments to the render functions:https://github.com/WordPress/gutenberg/blob/a6908dcc4cde747ee2879317597f73179bf6e4d1/packages/components/src/dropdown/index.js#L82
Instead of relying on so many different ways to control the state, we can make semi-controlled components similar to how React deals with form inputs:
This makes the
renderToggle
andrenderContent
props obsolete as we wouldn't need them to access the internal state anymore and the content could just be passed as children.popoverProps
popoverProps
would remain the same.Menu
(orDropdownMenu
)Currently, the
DropdownMenu
has acontrols
prop. I suggest we rename the component toMenu
to match therole="menu"
attribute and deprecate thecontrols
prop in favor ofMenuItem
s passed as children. ButDropdownMenu
would work as well.The MenuItem component already exists and follow the same API as
Button
, so we don't need to change anything here aside from connecting it to theMenu
component (through React Context) to provide a roving tabindex navigation similar to the Toolbar component.The snippet above would be rendered to the DOM as something like this (simplified):
Sub menus
Because
Menu
would useDropdown
, andDropdown
andMenuItem
has similar APIs, we could makeMenu
aware of parent menus and render it as a sub menu:Toolbar
Finally, we can combine all those components together in the toolbar. We should provide components like
ToolbarDropdown
andToolbarMenu
that would render the toggle button as a toolbar item automatically:The Toolbar component already follows this API, so we don't need to change many things there.
How to connect those components to
Toolbar
?The components would be connected through the headless ToolbarItem component that already exists.
ToolbarItem
passes down all the props necessary to render a toolbar item while being agnostic about which component will be rendered (it doesn't require it to be aButton
, for example). Here's an example of how theToolbarDropdown
component could be created:This is possible because we would pass the toolbar item props down to the toggle button on the
Dropdown
component. We can leverage the same technique if we want to connect other components too. For example, we may have a headlessMenuItem
component (maybe with another name to not conflict with the existingMenuItem
) to make any component a menu item.