-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
A consistent component API for @wordpress/components #33391
Comments
🤩 The As far as 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 |
I think it's similar to I've also researched about having a generic <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 React Spectrum takes a different approach though. It also doesn't allow us to change the structure of the elements inside a collection. You can only render |
@youknowriad In the examples, But I don't really have strong opinions on this. I also understand it may not be really obvious that But I've seen a lot of people confused about what component they should use, for example, in a When combining two different modules such as 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 |
In Gutenberg we do have |
I think I'm ok with not having a non-dropdown |
Yes! If we want a more generic menu (like a navigation menu that's always visible), we would use components like |
We can keep using <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 I personally like the rename to just |
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:gutenberg/packages/components/src/dropdown/index.js
Line 82 in a6908dc
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.The text was updated successfully, but these errors were encountered: