WP components audit: Button Group & Toolbar

[davewhitley]

Is your feature request related to a problem? Please describe.

This issue is part of a review on the naming, structure, and composition of the UI components as brought up in #16367

Button Group

Audit

Opportunities I came across while reviewing this component.

• Based on the name, I expected a simple group of buttons, each with its own action.
• Based on the documentation it looks like there should be some toggle functionality, but the component has none.
• Based on the name, I should be able to group icon buttons together, but see no way of doing that.

Grouping

• I expect this component to live in a Buttons category, instead of a top level item.

Suggestions

Structure (no changes):

ButtonGroup
─ Button
─ Button
─ Button

• Keep the name ButtonGroup
• Add toggle functionality: exclusive (radio) and multiple (checkbox). Default would be no toggling as it works currently.
• Add ability to group icon buttons together to create a "toolbar" component.

Note

The radio option of the ButtonGroup works similarly to TabPanel.

To clarify the difference between TabPanel and this ButtonGroup component — a tab represents a single view in a group of related views. A radio ButtonGroup represents a choice within a view. In other words, tabs are a navigational element, and ButtonGroup is not. Related: #13587

Toolbar

This component is included because of its close relation to ButtonGroup.

Audit

Opportunities I came across while reviewing this component.

• The basic functionality of this component could be generalized into something more reusable.

Grouping

• The Toolbar component, as it exists right now, could still exist as an editor component, but I would not expect it to live in general UI components.

Suggestions

• To create a group of toggle buttons (icon or text) we should use the ButtonGroup component.
• Move this more complex Toolbar component to an editor set of components.

Alternatively, we could have 2 components:

• ButtonGroup: Just a simple group of buttons with no toggle functionality.
• ToggleButtonGroup: Similar to ButtonGroup, but with toggle options.

Feedback

The feedback I'm looking for is related to naming, structure, and composition. I'm not looking for visual feedback of the components. Prop audit can be seen in the comment below.

• What do you think of combining the functionality into one component (toggle and no toggle)?
• What do you think about generalizing the Toolbar component into ButtonGroup?
• Do you think it makes sense that Toolbar is a specific component that would be mostly used in an editor context?

[youknowriad]

Thanks for the audit.

A few questions that comes to my mind based on the suggestions above:

Based on the documentation it looks like there should be some toggle functionality, but the component has none.

The toggle functionality is already available in the Button component that should be used as a children of ButtonGroup. Does this addresses that point or not? Having it on the Button allows the user to have both multiple or single toggled button addressed with the same API.

Add ability to group icon buttons together to create a "toolbar" component.

I guess I have the same question as above. It's possible to use an IconButton instead of a Button as a child of ButtonGroup. Does it address that point? If not, why?

Toolbar

As it exists today, it's basically just a ButtonGroup with a different API and a more opinionated design. It also supports "isCollapsed" that allows to switch between a regular toolbar and a Dropdown menu (since we need to be able to switch the way the toolbar is displayed in block toolbars depending on the context: space available, nesting...). It's not clear to me how do you propose to update these?

[davewhitley]

Thanks for taking a look!

The toggle functionality is already available in the Button component that should be used as a children of ButtonGroup. Does this addresses that point or not?

Partially. Is the isToggled' prop just a visual "pressed" style for Button`? That helps the issue a little bit. As far as I can tell, there's no option to create a button group with exclusive or multiple selection (as shown in the mockup) which is included when I said "toggle functionality".

I guess I have the same question as above. It's possible to use an IconButton instead of a Button as a child of ButtonGroup. Does it address that point? If not, why?

Yes, as long as my point about the toggle functionality is addressed above, then I think that solves the issue.

Toolbar...It's not clear to me how do you propose to update these?

I'm suggesting that Toolbar, as it currently exists, is not appropriate for a base set of UI components, (similar to FocusPicker). The Toolbar can be generalized for UI that does not need the extra functionality that you describe. For example, this UI is in the sidebar for one of the blocks:

I would classify the above UI as a ButtonGroup with IconButtons as children. I assume it's actually a Toolbar. I propose we:

• Update ButtonGroup with more toggle functionality.
• If possible, create a new component package, probably called @wordpress/components-editor and move Toolbar there. To me this makes the most sense.
• If the above is not possible, then create a separate folder within the components package.

[youknowriad]

As far as I can tell, there's no option to create a button group with exclusive or multiple selection (as shown in the mockup) which is included when I said "toggle functionality".

That's up to the user basically because it just depends on how the state is setup since all of our components are "controlled" components.

Exemple: (Exclusive)

const ExclusiveButtonGroupUsage = () => {
   const [ toggled, setToggled ] = useState( null );
   const items = [ /* some array */ ];

   return (
      <ButtonGroup>
            { items.map( item => (
                 <Button 
                      key={item.id} 
                      isToggled={toggled === item.id}
                      onClick={() => setToggled(  toggled === item.id ? null: item.id )}
                  >
                       {item.caption}
                  </Button>
      </ButtonGroup>
   );
}

Multiple:

const MultipleButtonGroupUsage = () => {
   const [ toggled, setToggled ] = useState( [] );
   const items = [ /* some array */ ];
   const toggle = ( id ) => {
      const isToggled = toggled.indexOf( if ) !== -1;
      if ( isToggled ) {
         setToggled( without( toggled, [ id ] ) );
      }   else {
         setToggled( [ ...toggled, id ] );
      }
   }

   return (
      <ButtonGroup>
            { items.map( item => (
                 <Button 
                      key={item.id} 
                      isToggled={toggled.indexOf( item.id ) !== -1}
                      onClick={() => toggle( item.id )}
                  >
                       {item.caption}
                  </Button>
      </ButtonGroup>
   );
}

What kind of API you're looking for?

Toolbar

I think there's something we need to clarify. It's not possible at this point to remove or rename components because we need to ensure Backward compatibility. What we can do is create new components or alias existing components (same component with two names and deprecate one of them but not remove it)

[davewhitley]

Controlled vs Uncontrolled is something I don't know enough about. I will do some digging on that. Thank you!

I believe what we want is something close to these 2 component libraries:

• https://material-ui.com/components/toggle-button/#toggle-buttons
• https://mineral-ui.com/components/button-group#mode

We should make it as easy as possible to include this functionality to lower the barrier for entry, and I believe it will create more consistent code, behavior, and functionality for everyone using these components.

What we can do is create new components or alias existing components (same component with two names and deprecate one of them but not remove it)

This sounds like the way forward

[davewhitley]

Props audit

As an addendum, here is a deeper evaluation of the props for these components. This is an effort to expand the components to be more useful and to answer the question, "Are properties well thought out and consistently applied?" This only covers visual props. Event handler props will be evaluated at a later date.

ButtonGroup

It seems there are no props for this component.

✨ New

• mode: accepts either radio(exclusive) or checkbox(multiple).
• variant: primary, default, or minimal (aligns with the same Button variants)
• fullWidth: 100% width of container.
• dense: sizing for all Button children.
• disabled: disable all Button children.

---

Toolbar

No suggestions at this point because this is a unique component for the editor and should be moved to an editor set of components.

---

Event handler props will be evaluated at a later date.

[diegohaz]

I'm working on making toolbars more accessible and hopefully fix #15331

But as I go, I see that @wordpress/components' Toolbar is not really a toolbar. It sounds more like a ResponsiveButtonGroup, since it seems to be a group of buttons that may be collapsed into a dropdown menu.

In practice, it's not even used as the block toolbar as the design guidelines indicate. The block toolbar uses NavigableContainer instead and the actual toolbar code lives in the block-editor components.

My initial thought was, then, to propose transforming @wordpress/components Toolbar into an actual toolbar and use it directly as a replacement for the navigable-toolbar. The current Toolbar would be renamed to something like ResponsiveButtonGroup.

But then I came across this: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/CONTRIBUTING.md#deprecation

At no point does the deprecated component/prop become unavailable.

What would be the best way to propose/make this change? A new separate module (e.g. AccessibleToolbar)?

cc @gziolo

[gziolo]

But as I go, I see that @wordpress/components' Toolbar is not really a toolbar. It sounds more like a ResponsiveButtonGroup, since it seems to be a group of buttons that may be collapsed into a dropdown menu.

Yes, totally agree that the current implementation of the Toolbar is wrong. We should replace all occurrences with something like ResponsiveButtonGroup or ToolbarGroup if we want to be consistent with MenuGroup.

My initial thought was, then, to propose transforming @wordpress/components Toolbar into an actual toolbar and use it directly as a replacement for the navigable-toolbar. The current Toolbar would be renamed to something like ResponsiveButtonGroup.

Yes, we should explore whether it's possible to refactor Toolbar to work like NavigableToolbar from the block editor package. In this scenario, in the block editor, we would have only a tiny wrapper over the general-purpose Toolbar.

We would also have to introduce another component to cover what we have today in the codebase. Ideally, we should also find a way to magically make the existing code work if someone still uses Toolbar as a grouping mechanism. I must admit this might be a huge challenge but maybe there is a way to detect it based on the props passed. One thing that might work is controls prop which in this case wouldn't exist in the new implementation of Toolbar. If we see it passed, we could render new components designed for toolbar button groups with the deprecation message.

Does it sound like a good approach for you? I'm happy to discuss all other alternatives. Honestly speaking, it's very challenging to ensure that old APIs continue to work while we reshape how components behave.

By the way, it seems like NavigableMenu should use only arrow keys to navigate between items and tab to the next component. At the moment tabbing and arrow keys work more or less the same. At least this is what I see in the Toolbar Example at w3.org:
https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html

cc @jameslnewell @ItsJonQ

[ItsJonQ]

We would also have to introduce another component to cover what we have today in the codebase...

@gziolo Thank you for sharing your thoughts! This approach is very reasonable, and it's nice to know that it's possible to make more drastic changes (from an API naming perspective), even if it may be challenging.

A wrapper/adapter acting as the "entry point" for the Toolbar, which than uses the newly build more semantically designed components proposed by @diegohaz makes sense to me.

+1 for deprecation messages regarding this approach

🎉

[gziolo]

@diegohaz works on improvements to the Toolbar component in #17875.

[mapk]

I'm closing out the Component Audit issues for now.