WP components audit: Button & IconButton

[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

Link vs Button

Please see this comment on how the link vs button discussion relates to this component library audit: #7534 (comment)

Button

Audit

Opportunities I came across while reviewing this component.

• There is no "Secondary" button, which is confusing because there is Primary and Tertiary.
• It appears that there is no default button (without props) but there is a Default button prop. Also confusing because it's actually the Secondary button style.
• I expected to be able to add an icon to this component (i.e. icon + text), which is a common pattern found in component libraries. I thought IconButton might be able to help, but it's icon-only.
• The isLink prop was confusing and problematic for me. Does it just style it as a link or does it actually use an anchor tag like in other component libraries?

Grouping

I expect Buttons to be a top level category containing several components including, but not limited to, ClipboardButton, IconButton, ButtonGroup, etc.

Suggestions

• Add option to display an icon. We'll need the ability to add a right aligned icon that indicate forward motion.
• Make the isDefault button style actually the default in the component. (e.g. <Button>Button</Button>). Even if it's not named "Default" it's still a good default style for the component. Other styles are used only if a button requires more or less importance.
• Remove isLink in favor of the Tertiary style since they look very similar (assuming there is no functional change). This makes more sense if we add a new Link component.
• Related to the above, if "href" is present, the component should use an anchor element instead of a button element.
• Improve naming. Each convention has its disadvantages. A disadvantage of semantic naming (our current approach) is that sometimes the primary action in a component might not use the Primary button style (e.g. a card might use the Default button for the primary action and the Tertiary button for the secondary action. Here are some options:
  • Primary, Secondary, Tertiary
  • Primary, Default, Minimal
  • Primary, Basic (or Default), Plain
  • Contained, Outlined, Text

IconButton

Audit

Opportunities I came across while reviewing this component.

• I wasn't sure what to expect based on the name of the component. I wasn't sure if it would be a icon + text button or an icon-only button component.
• There doesn't seem to be any toggle functionality in the component. For example, toggling an empty heart icon into a filled heart icon.

Grouping

I expect IconButton to live in a Buttons category, instead of a top level item.

Suggestions

• Add toggle functionality since it's a common pattern in component libraries.
• Based on this effort to show text labels for icon buttons, there seems to be less difference between IconButton and Button.

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.

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 two components?
• Have you encountered a successful naming convention for buttons (primary, secondary, etc.)?
• Have any additional thoughts on the isLink prop?

[mtias]

Thanks for doing this!

If we end up grouping all the components that are related to buttons together, I'm less concerned with having Button and IconButton as separate components, as there would be a single place to discover them. I agree with your take 1 there.

With things like #15830 I feel it's going to be quite convoluted to support all the variations in a single component. Also, it's going to be harder to say prop A cannot be used with prop B, and people would need to combine the right elements which seems prone to more errors.

We can consider renaming Button to TextButton.

Open to this as well.

I'm also not examining each prop for each component

I think it'd be good to do this, because it might help consider all the implications involved — can we have an isBusy icon button? What about an isDestructive? Would an isLink icon button make sense? What's the point of "large" and "small"? Does it impact secondary / tertiary semantic weight?

Add toggle functionality since it's a common pattern in component libraries.

We already have an isToggled prop, is this sufficient to determine toggle behaviour?

[mtias]

@drw158 also related #11937

[davewhitley]

... can we have an isBusy icon button? What about an isDestructive? Would an isLink icon button make sense? What's the point of "large" and "small"? Does it impact secondary / tertiary semantic weight?

If we go with separate components, would it help to determine which props are incompatible with each other? Is there a way to enforce that in code or just in guidelines? I could take a look at some other systems to see how that is handled.

We can consider renaming Button to TextButton.

Some disadvantages I see:

• Might be strange to not have a regular Button component as is standard with all component libraries.
• TextButton and IconButton sort of implies that TextButton can't have an icon, but I realize that's already true with IconButton for text.

We already have an isToggled prop, is this sufficient to determine toggle behaviour?

I couldn't tell if it worked in the environment I'm using, but yep that should work if both components have it. It would be nice if you could change the icon on toggle for Icon Button (e.g. empty star to filled star).

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

Button

isPrimary, isDefault, isTertiary

I don't see why any of these would need to be used in conjunction with one another, so I propose they become mutually exclusive.

• appearance or variant: accepts either primary, default, or minimal.

isLarge, isSmall

Two directions we could take. 1) Only offer one size, other than the default. 2) Offer 3 sizes, like we do now.

1. dense: makes the button smaller, for denser layouts
2. size: accepts either large, default, or small

Although offering 3 sizes is a common practice for component libraries, it can be unclear how and when to use these just by looking at the prop names (e.g. "Should I use large for tertiary buttons? What if that makes them bigger than primary buttons?). I suggest we offer only two size options, default and dense. It's more clear what the size is used for (more dense layouts), and the difference is negligible between our current sizes. I think we can consolidate:

We can also use the dense prop for other elements like selects, text fields, tables, etc. For making very large buttons, which should be uncommon, we could provide a way to add padding.

✨ New

• icon: useful for showing a leading icon
• iconEnd or trailingIcon: useful for showing a trailing icon if forward or expand motion is suggested (forward arrows, dropdown caret, etc.)
• fullWidth: 100% width of container
• theme: not sure how this should be done technically, but there's an opportunity to change token design elements and allow 3rd parties to theme buttons.

📋 Rename

• isToggled → "pressed" (if consistency with aria is preferred) or "selected" or "active". "toggled" would work as well. Salesforce has some documentation on their button states that might be useful.
• isBusy → loading
• isDestructive → danger or destructive (not all dangerous actions are destructive though, like installing a malicious plugin)

Removing "is" simplifies and removes redundancy.

👍 No change

• disabled
• href

✂️ Remove

• isLink: It's inadvisable to make a button look like a link. One of the 3 button types should be used instead.

---

IconButton

tooltip, label

The text here should be the same, so we should refactor how these are currently implemented:

• showLabel: shows the aria label as text, centered underneath the icon (e.g. Added show icon labels option #15830). Tooltip is hidden unless there's a shortcut defined.
• hideTooltip: if you need to hide the tooltip
• label: the aria label for the button. Can be shown using showLabel.
• ❓ labelPosition: couldn't figure out what this does

✨ New

All of the above props for Button should also apply for IconButton except for:

• iconEnd
• href
• ❓fullWidth: not sure if this would be needed

👍 No change

• icon
• shortcut

✂️ Remove

• children: I don't see the need to have children unless we wanted to support adding the icon as a child in addition to a prop.

---

Event handler props will be evaluated at a later date.

[youknowriad]

I think with the last button refactorings we have now a single Button component that supports icons, three variations (primary, secondary, tertiary) and support two sizes (small, default). I think the component is consistent enought that we can consider this issue closed.

There might be some things that can be improved (link buttons) but let's follow-up with more specific issues now.

[rmorse]

I just wanted to check in on this, I see that isLarge is documented, but has no CSS styles.

However, @youknowriad I see you mention having 2 sizes as best.

I see code in the component for isLarge and attaching an is-large class to the button.

What would be the best course of action?

1. Remove isLarge from docs and component
2. Add CSS for is-large
3. Open a new issue?

I came across this today and thought it would be a nice way to make my first code contribution :)

Thanks

[youknowriad]

Remove isLarge from docs and component

Seems like the path forward as it was an intended change. Thanks for your help.