Skip to content
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

Open
Tracked by #34304
diegohaz opened this issue Jul 13, 2021 · 10 comments
Open
Tracked by #34304

A consistent component API for @wordpress/components #33391

diegohaz opened this issue Jul 13, 2021 · 10 comments
Labels
[Package] Components /packages/components [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@diegohaz
Copy link
Member

diegohaz commented Jul 13, 2021

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 (or Flyout)

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 the anchorRect or getAnchorRect props.

We also have the Dropdown component, which uses the generic Popover underneath, but also renders a toggle button via the renderToggle prop.

Finally, we're experimenting with a Flyout component that looks similar to Popover and Dropdown, but with a completely different API. The goal here is to find a concise API similar to the Dropdown component API (with very few deprecations), but, ideally, we would use the Flyout implementation.

<Dropdown label="Label" icon={ icon }>
    <p>Content</p>
</Dropdown>

Aside from custom Dropdown props, all the props passed to the Dropdown component, including label and icon, would be passed over to the internal toggle button, which would render a Button by default. This makes the toggleProps prop obsolete.

The snippet above would be rendered to the DOM as something like this (simplified):

<button id="button" aria-label="Label"><svg /></button>
<div role="dialog" aria-labelledby="button">
    <p>Content</p>
</div>

Controlled components

Currently, we have different ways to control the state of the Popover components. The generic Popover component has an onClose prop. Dropdown has onClose and onToggle.

Dropdown also accepts render props like renderToggle and renderContent that pass isOpen, onClose and onToggle as arguments to the render functions:

const args = { isOpen, onToggle: toggle, onClose: close };

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:

const [ isOpen, setIsOpen ] = useState( false );

<Dropdown text="Visible label" isOpen={ isOpen } onToggle={ setIsOpen }>
    <p>Content</p>
</Dropdown>

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.

popoverProps

popoverProps would remain the same.

<Dropdown popoverProps={ { className: 'my-popover' } } />

Menu (or DropdownMenu)

Currently, the DropdownMenu has a controls prop. I suggest we rename the component to Menu to match the role="menu" attribute and deprecate the controls prop in favor of MenuItems passed as children. But DropdownMenu would work as well.

<Menu label="Label" icon={ icon }>
    <MenuItem label="Label" icon={ icon } />
    <MenuItem label="Label" icon={ icon } />
    <MenuItem label="Label" icon={ icon } />
    <MenuItem label="Label" icon={ icon } />
</Menu>

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 the Menu 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):

<button id="button" aria-label="Label"><svg /></button>
<div role="menu" aria-labelledby="button">
    <button role="menuitem" aria-label="Label"><svg /></button>
    <button role="menuitem" aria-label="Label"><svg /></button>
    <button role="menuitem" aria-label="Label"><svg /></button>
    <button role="menuitem" aria-label="Label"><svg /></button>
</div>

Sub menus

Because Menu would use Dropdown, and Dropdown and MenuItem has similar APIs, we could make Menu aware of parent menus and render it as a sub menu:

<Menu label="Label" icon={ icon }>
    <MenuItem label="Label" icon={ icon } />
    <MenuItem label="Label" icon={ icon } />
    <MenuItem label="Label" icon={ icon } />
    <Menu label="Label" icon={ icon }>
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
    </Menu>
</Menu>

Toolbar

Finally, we can combine all those components together in the toolbar. We should provide components like ToolbarDropdown and ToolbarMenu that would render the toggle button as a toolbar item automatically:

<Toolbar label="Label">
    <ToolbarButton label="Label" icon={ icon } />
    <ToolbarButton label="Label" icon={ icon } />
    <ToolbarDropdown label="Label" icon={ icon }>
        <p>Content</p>
    </ToolbarDropdown>
    <ToolbarMenu label="Label" icon={ icon }>
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
        <MenuItem label="Label" icon={ icon } />
    </ToolbarMenu>
</Toolbar>

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 a Button, for example). Here's an example of how the ToolbarDropdown component could be created:

function ToolbarDropdown( props, ref ) {
  return <ToolbarItem as={ Dropdown } ref={ ref } { ...props} />;
}

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 headless MenuItem component (maybe with another name to not conflict with the existing MenuItem) to make any component a menu item.

@diegohaz diegohaz added [Package] Components /packages/components [Type] Discussion For issues that are high-level and not yet ready to implement. labels Jul 13, 2021
@sarayourfriend
Copy link
Contributor

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?

@youknowriad
Copy link
Contributor

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?

@diegohaz
Copy link
Member Author

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?

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 useEffects 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.

@diegohaz
Copy link
Member Author

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?

@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?

@youknowriad
Copy link
Contributor

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?

@youknowriad
Copy link
Contributor

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.

@youknowriad
Copy link
Contributor

youknowriad commented Jul 13, 2021

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.

@diegohaz
Copy link
Member Author

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?

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.

@diegohaz
Copy link
Member Author

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.

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>

@mtias
Copy link
Member

mtias commented Jul 23, 2021

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 MenuItems 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

No branches or pull requests

4 participants