Merge the Button and IconButton into a single component #19193

youknowriad · 2019-12-17T08:20:43Z

See the Button & IconButton audit #16541 and the following part in particular:

Two options for Button and IconButton:

1. Keep Button and IconButton separate components

Now that it seems like both components will have text and icon options in the component, here would be the main differences:

They handle the text label differently. Button is primarily for text (with an option for a supplementary icon) and IconButton is primarily for an Icon (with an option for supplementary text).
For IconButton, text is hidden by default. When text is shown, it’s centered underneath the icon. When text is not shown, it's visible as a hover tooltip. There's always an icon shown.
For Button, text is shown by default. There's always text shown. When an icon is shown, it's aligned to the left. There's no need for a tooltip because the text is always shown.
We simplify the components by automatically handling the text label. The label is always the same whether it's shown or not. The components automatically handles the tooltip.
We can consider renaming Button to TextButton.

2. Combine Button and IconButton

It could be a simpler experience for designers/developers if we combined the two components. Here is an example of a component library that does this.

The main advantage is that the user of the system won't have to figure out which component to use, since it's all-in-one. The disadvantage is that it might be complex figuring out the different props, and the combination of certain props (e.g. isPrimary + isIconOnly). We might have to split the two components for technical reasons.

In this PR, I'm trying the second approach here and unify the Button and IconButton. Code-wise, it doesn't seem complex. The questions if we follow that route are:

Should we update all of our existing usage?
Should we officially deprecate the IconButton component?

Thoughts @mtias @davewhitley @gziolo @aduth @jasmussen @ItsJonQ @diegohaz

gziolo · 2019-12-17T08:41:27Z

For IconButton, text is hidden by default. When text is shown, it’s centered underneath the icon. When text is not shown, it's visible as a hover tooltip. There's always an icon shown.

For Button, text is shown by default. There's always text shown. When an icon is shown, it's aligned to the left. There's no need for a tooltip because the text is always shown.

I think it's fine to keep IconButton based on the description provided above. I guess we could apply some more refactorings to make it more evident:

IconButton shouldn't allow children because it already uses an icon as a child element
Button should not introduce tooltip but rather continue using label but use another prop to detect whether the tooltip should show up - tooltipPosition maybe, or label + labelPosition + hasTooltip? The idea would be to support the following when an icon is present:
- text aligned to the left
- text aligned to the right
- small label under
- small label above
- all positions when tooltip enabled

youknowriad · 2019-12-17T08:48:14Z

IconButton shouldn't allow children because it already uses an icon as a child element

We do support buttons with an icon and a text at the same time, we shouldn't break this behavior.

Button should not introduce tooltip but rather continue using label but use another prop to detect whether the tooltip should show up - tooltipPosition maybe.

The reason we have two props right now is because the aria-label and the tooltip text can be different. I agree that ideally one should be enought for both but we should check existing usage before making that change.

It seems like you're saying we can't have "children" (text) and tooltip at the same time. Well! maybe we do? but I believe we need some design work if we want to support things llike "icon on the left + text", "icon on the right + text", "icon above text", "icon under text", regardless of whether these buttons are tooltips. These use-cases should be implemented separately once we have some clear design directions and use-cases. For the moment, I tried focusing on the existing components and their capabilities and how we can merge them together.

gziolo · 2019-12-17T09:06:51Z

IconButton shouldn't allow children because it already uses an icon as a child element

We do support buttons with an icon and a text at the same time, we shouldn't break this behavior.

To clarify, I'm not saying we should stop doing that if it exists, but we should make it clear that IconButton isn't designed to work with children. It's an icon with some labels. So maybe, we can keep the behavior but discourage it with deprecation? It can be a separate exploration though.

The reason we have two props right now is because the aria-label and the tooltip text can be different. I agree that ideally one should be enought for both but we should check existing usage before making that change.

I missed that, it's probably not ideal. I remember reading about patterns related to that in https://inclusive-components.design/tooltips-toggletips/. I don't know anymore what is the best way of moving forward 😅 It needs some further investigation of what would be the best way to handle it. Reakit uses aria-describedby rather than aria-label and this is also what's recommended in the linked book.

jasmussen · 2019-12-17T09:24:32Z

Took this PR for a spin, and things look unchanged in the editor. That's good! Also, From a visual/design point of view, this unification does make sense.

On a higher level, and speaking not of changes in this branch but of changes to come in the future, I would caution being too principled with the properties, as things like paddings will diverge depending on whether a button has an icon or not, and indeed a number of other aspects.

For example, icon only buttons should be square:

... unless multiple icon only buttons are shown in a row, in which case their dimensions need to be optically balanced. Here, the red shows 48x48px rectangles and how they overlap to be optically balanced. The blue shows that buttons other than the first and last in a sequence actually need to be 36x48:

Buttons themselves also need a wider horizontal padding in order to look harmonious, but more importantly, to distance themselves from input fields:

And lastly, icon plus text buttons will likely need horizontal padding that is different on the left vs. on the right, so as to balance the icon correctly:

None of that should be impossible to do. But it is something we should be aware of.

packages/components/src/button/index.js

aduth · 2019-12-17T13:22:18Z

Previously: #7534, #9702

Notably, despite #7534 seemingly being a contradictory proposal, there were similar conclusions reached (#7534 (comment)).

aduth · 2019-12-17T13:24:10Z

packages/components/src/button/index.js

@@ -44,19 +58,56 @@ export function Button( props, ref ) {
 'is-busy': isBusy,
 'is-link': isLink,
 'is-destructive': isDestructive,
+ 'has-text': !! icon && !! children,
+ // Ideally should be has-icon but this is named this way for BC


I thought classes didn't qualify?

https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/backward-compatibility/README.md#class-names-and-dom-updates

Yes, we can do it, just trying to keep the changes small for now. Ideally, we also deprecate the component.

I looked at this and there are a few components that rely on this for styling. I'm going to leave it for a follow-up PR to ensure we test these properly and ease review of the current PR.

youknowriad · 2019-12-18T09:57:08Z

I noticed that the "tooltip" prop is only used to force showing a tooltip or to disable it and not necessarily to change the tooltip text in our code base.
I think it's better to be explicit and use a "showTooltip" prop instead and always use the "label" as the text. See 2ca5ed5

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

youknowriad · 2019-12-19T13:53:48Z

any thoughts on landing this?

I have two follow-ups planned:

Remove the components-icon-button className.
Deprecate officilaly IconButton and replace its usage by just Button.

gziolo · 2019-12-19T14:04:08Z

packages/components/src/button/index.js

+ // the children are empty and...
+ ( ! children || ( isArray( children ) && ! children.length ) ) &&
+ // the tooltip is not explicitly disabled.
+ false !== showTooltip


this condition is now very similar to

( showTooltip && label )

we can refactor it later as it's quite hard to read :)

There's a difference between explicit disabling of tooltips and "undefined"

I figured it out after 2 additional minutes spend on contemplating on it 🤣

gziolo

Let's move forward, I love the direction where it is heading. It's easier to review when we do it in smaller steps 👍

jorgefilipecosta · 2020-02-11T15:13:19Z

Removing the needs dev note label because I think the dev note for #19299 will also handle this change.

dgwyer · 2020-05-27T17:11:01Z

@youknowriad Are there any plans to include this in WP core yet? I mistakenly thought that it was already as I'm using it my plugins but noticed it broke when I deactivated the GB plugin.

dgwyer · 2020-05-27T17:31:08Z

My bad, had wrong version installed. I see it now. Looks great!

youknowriad requested review from ajitbohra, chrisvanpatten, jorgefilipecosta and talldan as code owners December 17, 2019 08:20

youknowriad added [Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement. labels Dec 17, 2019

youknowriad self-assigned this Dec 17, 2019

gziolo reviewed Dec 17, 2019

View reviewed changes

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

diegohaz mentioned this pull request Dec 17, 2019

Components: Improve ToolbarButton #18931

Merged

6 tasks

aduth reviewed Dec 17, 2019

View reviewed changes

aduth mentioned this pull request Dec 18, 2019

Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

Closed

5 tasks

youknowriad and others added 4 commits December 19, 2019 13:55

Merge the Button and IconButton into a single component

82a286e

Fix showTooltip

c2a8dfa

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

Replace tooltip prop with showTooltip

248c498

Fix unit tests

d8dac0b

youknowriad force-pushed the update/icon-button branch from 2ca5ed5 to d8dac0b Compare December 19, 2019 13:25

youknowriad requested a review from ellatrix as a code owner December 19, 2019 13:25

youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Dec 19, 2019

Fix e2e snapshots

d0a9819

youknowriad requested review from nerrad and ntwb as code owners December 19, 2019 13:48

gziolo reviewed Dec 19, 2019

View reviewed changes

gziolo approved these changes Dec 19, 2019

View reviewed changes

youknowriad merged commit 2237573 into master Dec 19, 2019

youknowriad deleted the update/icon-button branch December 19, 2019 14:13

youknowriad mentioned this pull request Dec 19, 2019

Remove components-icon-button classname #19241

Merged

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

jorgefilipecosta removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 11, 2020

jorgefilipecosta added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 11, 2020

jorgefilipecosta mentioned this pull request Feb 12, 2020

Block Editor Dev Notes 5.4 #20185

Closed

23 tasks

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020

gziolo mentioned this pull request Dec 4, 2020

Implementing a Component System #27484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge the Button and IconButton into a single component #19193

Merge the Button and IconButton into a single component #19193

youknowriad commented Dec 17, 2019

gziolo commented Dec 17, 2019

youknowriad commented Dec 17, 2019

gziolo commented Dec 17, 2019

jasmussen commented Dec 17, 2019

aduth commented Dec 17, 2019

aduth Dec 17, 2019

youknowriad Dec 17, 2019

youknowriad Dec 18, 2019

youknowriad commented Dec 18, 2019

youknowriad commented Dec 19, 2019

gziolo Dec 19, 2019

youknowriad Dec 19, 2019

gziolo Dec 19, 2019

gziolo left a comment

jorgefilipecosta commented Feb 11, 2020

dgwyer commented May 27, 2020

dgwyer commented May 27, 2020

Merge the Button and IconButton into a single component #19193

Merge the Button and IconButton into a single component #19193

Conversation

youknowriad commented Dec 17, 2019

Two options for Button and IconButton:

gziolo commented Dec 17, 2019

youknowriad commented Dec 17, 2019

gziolo commented Dec 17, 2019

jasmussen commented Dec 17, 2019

aduth commented Dec 17, 2019

aduth Dec 17, 2019

Choose a reason for hiding this comment

youknowriad Dec 17, 2019

Choose a reason for hiding this comment

youknowriad Dec 18, 2019

Choose a reason for hiding this comment

youknowriad commented Dec 18, 2019

youknowriad commented Dec 19, 2019

gziolo Dec 19, 2019

Choose a reason for hiding this comment

youknowriad Dec 19, 2019

Choose a reason for hiding this comment

gziolo Dec 19, 2019

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Feb 11, 2020

dgwyer commented May 27, 2020

dgwyer commented May 27, 2020