Components: Improve ToolbarButton by diegohaz · Pull Request #18931 · WordPress/gutenberg

diegohaz · 2019-12-05T05:30:08Z

Description

This PR is part of #18619. Please, refer to the issue for more context.

I confess that coming up with a good API here is really challenging, considering all the use cases in Gutenberg. So I'd really appreciate any feedback on that. The main use cases I identified and am trying to address with this PR are:

ToolbarButton

import { Toolbar, ToolbarButton } from "@wordpress/components";

<Toolbar __experimentalAccessibilityLabel="label">
  <ToolbarButton>Button 1</ToolbarButton>
  <ToolbarButton icon="icon">Button 2</ToolbarButton>
</Toolbar>

Button supports icons since Merge the Button and IconButton into a single component #19193. ToolbarButton passes all the props down to Button, so it also supports it.

ToolbarItem (generic headless component for any other kind of toolbar item)

import { Toolbar, __experimentalToolbarItem as ToolbarItem } from "@wordpress/components";

<Toolbar __experimentalAccessibilityLabel="label">
  <ToolbarItem>
    {toggleProps => (
      <DropdownMenu
        icon="icon"
        label="label"
        controls={ ... }
        toggleProps={ toggleProps }
      />
    )}
  </ToolbarItem>
</Toolbar>

Eventually, if this is common enough, we can create something like ToolbarDropdownMenu to abstract the usage above. But for this first iteration I thought that Button and IconButton would be enough. The dropdown use case can also use <ToolbarGroup isCollapsed>, but some usages in Gutenberg (and in third party blocks, I suppose), can't be easily converted into ToolbarGroup due to styling conflicts.

There are other complicated cases like BlockSwitcher, which uses Dropdown directly with custom configuration. ToolbarItem could be used for that as well.

All the new features have been added as __experimental.

How has this been tested?

Check Storybook

Screenshots

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

packages/components/src/toolbar-button/index.js

packages/components/src/toolbar-button/style.scss

packages/components/src/toolbar-icon-button/index.js

packages/components/src/toolbar-item/index.js

diegohaz · 2019-12-26T02:58:51Z

packages/components/src/toolbar-button/index.js

 function ToolbarButton( {
 	containerClassName,
-	icon,
-	title,
-	shortcut,
-	subscript,
-	onClick,
 	className,
-	isActive,
-	isDisabled,
 	extraProps,
 	children,
+	...props


Removed most of the props here so we can spread { ...props } directly into Button. This way, ToolbarButton (when used within <Toolbar __experimentalAccessibilityLabel="label">) and Button have identical API.

diegohaz · 2019-12-26T03:01:25Z

packages/components/src/toolbar/test/index.js

-					<ToolbarButton title="control1" />
-					<ToolbarButton title="control2" />
+					<ToolbarButton label="control1" />
+					<ToolbarButton label="control2" />


When used within <Toolbar __experimentalAccessibilityLabel>, ToolbarButton passes all the props down to Button so they have the same API.

gziolo

I still need to do some testing with Storybook, but it is definitely looking very good. I like where it's all heading making it so much simpler to build toolbar elements.

ToolbarItem gives a lot of flexibility and should cover the majority of edge cases. We still should revisit the collapsed groups and offer some easier way to code the dropdown menu there. It's big enough to tackle it separately though after we refactor all the existing toolbars. Once it's done, it should be easier to come up with a good API.

gziolo · 2020-01-03T10:30:48Z

packages/components/src/toolbar-button/index.js

+					icon={ props.icon }
+					label={ props.title }
+					shortcut={ props.shortcut }
+					data-subscript={ props.subscript }


I will investigate in a follow-up whether we can get rid of subscript as I think we no longer use it in code and it probably was never meant to exist as public API.

I kept it there so as to avoid breaking anything, but it's not being used in the new implementation (using ToolbarItem) anyway.

gziolo · 2020-01-03T10:39:41Z

packages/components/src/toolbar-button/index.js

+			<ToolbarButtonContainer className={ containerClassName }>
+				<Button
+					icon={ props.icon }
+					label={ props.title }


Does it mean that title should be deprecated in favor of label?

I'd say that these props should be deprecated:

title (in favor of label)

isActive (in favor of isPressed)

isDisabled (in favor of disabled)

Still not sure what to do with isDisabled though.

This PR only changes the behavior of ToolbarButton when it's used inside <Toolbar __experimentalAccessibilityLabel>, so if we decide to deprecate those props, I guess it should be in another PR.

I like the idea, let's go this way in the follow-up PRs.

gziolo · 2020-01-03T10:40:27Z

packages/components/src/toolbar-button/index.js

+						}
+					} }
+					className={ classnames( 'components-toolbar__control', className ) }
+					isPressed={ props.isActive }


A similar question, should we align API and deprecated isActive in favor of isPressed?

In stories, I still can see isActive:

That's the current API. This PR only changes ToolbarButton when used within <Toolbar __experimentalAccessibilityLabel>.

Right, we will need to align it at some point with the Button component as discussed.

gziolo · 2020-01-03T10:43:02Z

packages/components/src/toolbar-button/index.js

+					} }
+					className={ classnames( 'components-toolbar__control', className ) }
+					isPressed={ props.isActive }
+					disabled={ props.isDisabled }


I think this was discussed in other PRs, we should make a final call on whether we use isDisabled in all places instead of disabled to potentially break the direct association with the HTML attribute.

/cc @youknowriad @aduth

I think it makes sense to adopt isDisabled. Specially if #19337 is merged as it'll not be the same as the HTML disabled prop.

gziolo · 2020-01-03T11:11:11Z

packages/components/src/toolbar/stories/index.js

-				<ToolbarButton icon="editor-paragraph" title="Paragraph" />
+				<ToolbarButton icon="editor-paragraph" label="Paragraph" />
+			</ToolbarGroup>
+			<ToolbarGroup>


Is there any difference between this and:

It looks like we have the collapsed group case covered :)

There are some cases in Gutenberg where we're using DropdownMenu directly instead <ToolbarGroup isCollapsed>. For example:

gutenberg/packages/block-editor/src/components/block-settings-menu/index.js

Lines 59 to 66 in 2c1e3be

<Toolbar>

<DropdownMenu

icon="ellipsis"

label={ __( 'More options' ) }

className="block-editor-block-settings-menu"

popoverProps={ POPOVER_PROPS }

>

{ ( { onClose } ) => (

I'm not sure how easy is it to refactor those edge cases to use <ToolbarGroup isCollapsed>, so I just wanted to make sure ToolbarItem can be used.

Cool, it all makes sense. I think I mentioned it already, I just wanted to ensure you are aware of the current state of the art :)

gziolo

It tests well. I guess, we can plan to merge it soon.

gziolo

We discussed the current state of this PR and future steps. Let's iterate quickly with smaller steps

mtias · 2020-01-07T17:17:41Z

Thanks for all the work here!

Initial refactor on ToolbarButton

badb51d

diegohaz requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners December 5, 2019 05:30

diegohaz mentioned this pull request Dec 5, 2019

Components: Improve Toolbar #18619

Closed

7 tasks

Update storyshots

31dcf58

diegohaz commented Dec 5, 2019

View reviewed changes

packages/components/src/toolbar-button/index.js Outdated Show resolved Hide resolved

gziolo added [Package] Components /packages/components [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement. labels Dec 5, 2019

gziolo assigned diegohaz Dec 5, 2019

Update ToolbarButton styles

e5110b1

diegohaz commented Dec 5, 2019

View reviewed changes

packages/components/src/toolbar-button/style.scss Outdated Show resolved Hide resolved

gziolo reviewed Dec 6, 2019

View reviewed changes

packages/components/src/toolbar-icon-button/index.js Outdated Show resolved Hide resolved

gziolo mentioned this pull request Dec 6, 2019

Components: Allow to apply isPressed prop to Button and SVG components to indicate the state of the button #17748

Merged

7 tasks

diegohaz added 4 commits December 11, 2019 10:06

Merge branch 'master' into update/toolbar-button

474aba5

Merge branch 'master' into update/toolbar-button

da487d9

Update snapshots

b3bf911

Remove unnecessary styles for components-toolbar-button

7779fa7

gziolo mentioned this pull request Dec 19, 2019

Accessibility: Make text alignment items radio menu items. #19233

Merged

6 tasks

gziolo reviewed Dec 19, 2019

View reviewed changes

packages/components/src/toolbar-item/index.js Show resolved Hide resolved

Merge branch 'master' into update/toolbar-button

0643f3c

diegohaz mentioned this pull request Dec 24, 2019

Add warning package #19317

Merged

6 tasks

diegohaz added 3 commits December 25, 2019 23:34

Merge branch 'master' into update/toolbar-button

482e28f

Update Toolbar Button imports

aeca4d6

Refactors

b3ef374

diegohaz commented Dec 26, 2019

View reviewed changes

diegohaz mentioned this pull request Dec 27, 2019

Components: Add isFocusable state to Button #19337

Merged

6 tasks

gziolo reviewed Jan 3, 2020

View reviewed changes

Merge branch 'master' into update/toolbar-button

83aeceb

gziolo approved these changes Jan 7, 2020

View reviewed changes

diegohaz merged commit b3048b1 into WordPress:master Jan 7, 2020

diegohaz deleted the update/toolbar-button branch January 7, 2020 16:22

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

This was referenced May 26, 2020

Add new Parent block selector to child blocks (mahjong) #21056

Merged

Deprecate ToolbarButton props #22637

Closed

sirbrillig mentioned this pull request Jan 4, 2022

Improve @wordpress/components types for Animate, DropdownMenu, ResizableBox, and ToolbarButton DefinitelyTyped/DefinitelyTyped#57965

Merged

8 tasks

	<Toolbar>
	<DropdownMenu
	icon="ellipsis"
	label={ __( 'More options' ) }
	className="block-editor-block-settings-menu"
	popoverProps={ POPOVER_PROPS }
	>
	{ ( { onClose } ) => (

Conversation

diegohaz commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

mtias commented Jan 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

diegohaz commented Dec 5, 2019 •

edited

Loading