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

Merge the Button and IconButton into a single component #19193

Merged
merged 5 commits into from Dec 19, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Member

gziolo commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Member

gziolo commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 17, 2019

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:

Screenshot 2019-12-17 at 10 15 52

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

70900323-ddb1f800-1ff8-11ea-9a80-3863fbc99899

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

Screenshot 2019-12-17 at 10 16 14

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:

Screenshot 2019-12-17 at 10 16 42

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

@diegohaz diegohaz mentioned this pull request Dec 17, 2019
6 of 6 tasks complete
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 17, 2019

Previously: #7534, #9702

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 17, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 18, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 18, 2019

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

youknowriad and others added 4 commits Dec 17, 2019
@youknowriad youknowriad force-pushed the update/icon-button branch from 2ca5ed5 to d8dac0b Dec 19, 2019
@youknowriad youknowriad requested a review from ellatrix as a code owner Dec 19, 2019
@youknowriad youknowriad requested review from nerrad and ntwb as code owners Dec 19, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 19, 2019

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.
// the children are empty and...
( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitly disabled.
false !== showTooltip

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 19, 2019

Member

this condition is now very similar to

( showTooltip && label )

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 19, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 19, 2019

Member

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

@gziolo
gziolo approved these changes Dec 19, 2019
Copy link
Member

gziolo left a comment

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

@youknowriad youknowriad merged commit 2237573 into master Dec 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/icon-button branch Dec 19, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.