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

Button guidance PR #4409

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

alina-jacob
Copy link
Member

@alina-jacob alina-jacob commented Dec 13, 2024

Closes:
#4397: Belongs to button component guidance umbrella
#4176: Belongs to danger icon-only button guidance umbrella

Important links:
Figma file


Context

  • Added button guidance as a part of the consolidation effort from PAL to Core.
  • Updated content of Usage and Style Tab to better accommodate guidance and follow the current templates.
  • [Note: No content change has been done for the Button groups section.]

@alina-jacob alina-jacob self-assigned this Dec 13, 2024
Copy link

vercel bot commented Dec 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
carbondesignsystem ❌ Failed (Inspect) Dec 19, 2024 7:01pm

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to finish reviewing but don't want to lose my comments so far. Update: I'm done reviewing. Its looking good though! Most of my comments are little things or on sections that were probably out of scope for your work but wanted to leave comments anyways.

@@ -84,13 +104,13 @@ communicate calls to action to the user and allow users to interact with pages
in a variety of ways. Button labels express what action will occur when the user
interacts with it.

#### When to use
### When to use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's probably not consistent with other pages but I liked these as H4s more. Their hierarchy is just clearer since they're such short sections (just an opinion here).

Comment on lines +138 to +141
A button’s label is the most important element on a button, as it communicates
the action that will be performed when the user interacts with it. In a button
the label is always left-aligned, not center-aligned. By default Carbon uses
sentence case for all button labels.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this section could use some rephrasing. Here's my suggestion (its also fine to keep as is).

A button’s label communicates the action that is performed when the user interacts with it. In an icon button, that has no label, the icon must accurately represent what the button does.

In a Carbon button, the label is always left-aligned, not center-aligned. The icons in a button with a label is right-aligned but in an icon button, the icon is centered. 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also recommend the sentence about sentence casing down to the content section instead.

| ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------- |
| Small | Use when vertical space is limited or in areas with a confined layout. |
| Medium | Use when buttons are paired with 40px medium sized input fields. |
| Large (productive) | This is the most common button size. Use 14px body copy. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| Large (productive) | This is the most common button size. Use 14px body copy. |
| Large (productive) | This is the most common button size in software products. Pairs with 14px body copy. |

| Small | Use when vertical space is limited or in areas with a confined layout. |
| Medium | Use when buttons are paired with 40px medium sized input fields. |
| Large (productive) | This is the most common button size. Use 14px body copy. |
| Large (expressive) | The larger expressive type size within this button provides balance when used with 16px body copy. Used by the IBM.com team in website banners. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| Large (expressive) | The larger expressive type size within this button provides balance when used with 16px body copy. Used by the IBM.com team in website banners. |
| Large (expressive) | The larger expressive type size within this button provides balance when paired with 16px body copy. Used by the IBM.com team in website banners. |

| Large (productive) | This is the most common button size. Use 14px body copy. |
| Large (expressive) | The larger expressive type size within this button provides balance when used with 16px body copy. Used by the IBM.com team in website banners. |
| Extra large | Use when buttons bleed to the edge of a larger component, like in the context of modals, side panel and narrow tearsheets. |
| 2XL | Use when buttons bleed to the edge of a larger component, like in the context of large tearsheets. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both XL and 2XL say "edge of larger components..." can we differentiate here a little more?

Suggested change
| 2XL | Use when buttons bleed to the edge of a larger component, like in the context of large tearsheets. |
| 2XL | Use when buttons bleed to the edge of a full-screen component, like in the context of large tearsheets. |

buttons that call the `$button-separator` token for borders. This feature adds a
3:1 distinction between the two interactive UI elements. The border is a
recommended feature to improve accessibility.
There is [a 1px border](/components/button/style#ghost-button-structure.mdx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this link correct? Its going to ghost button structure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing the inline link, maybe do the "See the style tab for details" type of thing at the end of the paragraph.


</DoDont>
</DoDontRow>

#### Internationalization (RTL)

For RTL (right-to-left) languages, the entire button is mirrored horizontally.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For RTL (right-to-left) languages, the entire button is mirrored horizontally.
For right-to-left (RTL) languages, the entire button is mirrored horizontally.

### States

The default and interactive states of buttons should follow the
button [Style](/components/button/style#color.mdx) tab guidelines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
button [Style](/components/button/style#color.mdx) tab guidelines.
button [Style](/components/button/style#color) tab guidelines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overlay of this button looks off. Maybe try it with the multiply effect.

[accessibility tab](/components/button/accessibility).
[Accessibility](/components/button/accessibility) tab.

### Validation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO I think we could cut this section since its basically a don't. It could be confusing to user.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, finished, awesome work @alina-jacob! Just smalls things. The style tab looked perfect, no notes there.

Comment on lines +785 to +786
action. [Button groups](#button-groups) contain more guidance on how to use
secondary buttons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase to fit our standard "See more" formatting

Suggested change
action. [Button groups](#button-groups) contain more guidance on how to use
secondary buttons.
action. See [Button groups](#button-groups) for more guidance on how to use
secondary buttons.


#### Tertiary buttons in button groups

In [Button groups](#button-groups), where there is one primary and two other
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this isn't directly reference the "Button group" heading it can just be lower case.

Suggested change
In [Button groups](#button-groups), where there is one primary and two other
In [button groups](#button-groups), where there is one primary and two other

- Glyphs should always appear to the right of the text
buttons.
- Glyphs are distinguished by their solid shape and knocked-out details.
- Glyphs should always appear to the right of the text.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we saying "Glyphs" here in all these bullet points, isn't this the rule for icons?

Do you think a universal action with a clearly defined icon is missing? Let us
know [here](https://github.com/carbon-design-system/carbon/issues/new/choose).

Icons that are not in this list can be used in buttons, as long as the icon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the sentence up with the other content above the table. These three paragraphs just feel very choppy here.
image

are often found in the UI left navigation area, side panel, cards, and modals.
The target destination of the launch action should be made clear to the user
through the button or link label and the surrounding context. For more guidance,
see [this section](https://pages.github.ibm.com/carbon/ibm-products/guidelines/content/navigation-labels/) 
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this link, can we be more specific instead of saying "this section"? It also link to an internal site so we need to put "For IBMers only:" at the beginning of the section

would launch a new page.
</Caption>

### Best practices
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a general "best practices"? Or best practices of icons in buttons? I'm not sure "Best practices should be an H3 under "Modifiers"

Icon only buttons should be used sparingly. “For most situations, users learn
correct interpretations better with text alone than with icons alone.” —
Wiedenbeck, S (1999). For this reason, using icon-only buttons is recommended
for the following use cases.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for the following use cases.
for the following use cases:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🚦 In Review
Development

Successfully merging this pull request may close these issues.

4 participants