Visitar URL original
Button/Icon: Improve default appearance of button with icon by aduth · Pull Request #4493 · uswds/uswds · GitHub
Skip to content

Button/Icon: Improve default appearance of button with icon - #4493

Closed
aduth wants to merge 3 commits into
uswds:developfrom
aduth:aduth-button-icon-layout
Closed

Button/Icon: Improve default appearance of button with icon#4493
aduth wants to merge 3 commits into
uswds:developfrom
aduth:aduth-button-icon-layout

Conversation

@aduth

@aduth aduth commented Feb 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request seeks to improve the default appearance of using an Icon within a Button.

Using an icon to accompany a button's text is a common pattern, one which is even recommended in the Icon usability guidance:

If the icon is part of an interactive element, it should be implemented within another functional component, like as part of a button or list.

Additional information

Slack discussion context: https://gsa-tts.slack.com/archives/C3F14AHSQ/p1643382119000500

Screenshots

Before After
Screen Shot 2022-02-07 at 1 29 11 PM Screen Shot 2022-02-07 at 1 23 47 PM

The goal with this implementation was to avoid requiring any additional markup to support using an icon in a button. Ideally, a developer should be able to insert the icon and it "just works".

A couple open questions worth considering:

  • Is _icon.scss the best file for these styles to reside? Or would a button stylesheet make more sense? Or is this something completely different?
  • It may be easy enough to support a suffixing icon as well as a leading icon, though perhaps it's reasonable for the design system to be opinionated about this decision.

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

aduth
aduth commented Feb 1, 2022
View reviewed changes
Comment thread src/stylesheets/components/_icon.scss Outdated
Comment on lines +28 to +29
margin-left: -0.5rem;
margin-right: 0.5rem;

@aduth aduth Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm open to feedback about the specific offsets here, since it's all a bit subjective.

@aduth aduth Feb 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm open to feedback about the specific offsets here, since it's all a bit subjective.

I adjusted this further in 5abd934 after some discussion with our in-team UX folks, based on differences in appearance across icons. Previously, the idea was to try to create a "centered" effect between the left edge of the button and the start of the text. With 5abd934, the idea is more to have the icon adjacent to the text, with a bit of margin. This is offset (negated) a bit based on some built-in padding to most icons, which appears to be at least 2px inside the default 24px viewport. The idea with the calculation in 5abd934 is to adjust this relative to the font size of the button, so that the spacing can increase a bit with larger buttons (e.g. the "Big" variant).

develop dd973bd (original proposal) 5abd934 (revised proposal)
Screen Shot 2022-02-07 at 1 29 11 PM Screen Shot 2022-02-07 at 1 28 13 PM Screen Shot 2022-02-07 at 1 23 47 PM

Comment thread src/stylesheets/components/_icon.scss Outdated
Comment on lines +22 to +23
margin-top: -0.5 * ($icon-inline-size-magic-number - lh($theme-button-font-family, 1));
margin-bottom: -0.5 * ($icon-inline-size-magic-number - lh($theme-button-font-family, 1));

@aduth aduth Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These margins exist to offset the height increase which would otherwise occur due to the icon. With default settings, a button defines a line-height of 0.9, but this doesn't apply to the SVG, so the idea is to correct it by subtracting the difference (0.1em) equally from the top and bottom of the icon (margin-top: -0.05em; margin-bottom: -0.05em;).

@aduth aduth mentioned this pull request Feb 2, 2022
Merged
@aduth

aduth commented Feb 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

One noteworthy consideration we're encountering as we're preemptively implementing these styles in our project: The float-ing of the icon works well as a way to avoid excess spacing between the icon and text when the HTML markup includes whitespace between the two, which is pretty easy to do inadvertently:

Screen Shot 2022-02-03 at 10 48 01 AM

However, if a button is stretched using a width utility (e.g. width-full, min-width), the float will cause the icon to appear at the far left of the button, rather than left of the centered text.

image

The float can be avoided (which is what we've done), but this means a developer must be careful to avoid whitespace themselves if they want the spacing to be correct.

@mejiaj
mejiaj self-requested a review February 9, 2022 15:18
@mejiaj mejiaj added the Needs: Discussion An unresolved decision blocks progress. Name the decision; remove this label when decided. label Jul 14, 2022
@mejiaj

mejiaj commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

Sorry for the late response. I think this is a needed improvement, but needs discussion on how to best handle the use cases. For example, any specific styles tied to icon being inside buttons might need it's own modifier class.

In the meantime I've tagged it for Needs discussion and will add @amyleadem and @thisisdano as reviewers.

@mejiaj
mejiaj requested review from amyleadem and thisisdano July 14, 2022 18:58
@aduth aduth mentioned this pull request Apr 28, 2023
Open
@mejiaj mejiaj removed the Needs: Discussion An unresolved decision blocks progress. Name the decision; remove this label when decided. label Jun 29, 2023
@mejiaj mejiaj mentioned this pull request Jun 30, 2023
Closed
2 tasks done
@mejiaj

mejiaj commented Jun 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Opened up an enhancement for this issue #5359. We want to look at potential solutions that:

  • Don't rely heavily on calculations for alignment/spacing.
  • Use simpler selectors (potentially :has()).
  • Include a separate story or control for this change.

We'll open a new PR against #5359 for this work.

@mejiaj

mejiaj commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

Thank you for the contribution and I apologize it took so long to address.

I've created a new PR to add basic alignment to icons in buttons without relying on calc(). In the future, I'd like to allow more control for alignment, control, theming. Hopefully on improved support for:has().

Closing in favor of #5398.

@mejiaj mejiaj closed this Jul 21, 2023
@aduth
aduth deleted the aduth-button-icon-layout branch January 29, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejiaj mejiaj Awaiting requested review from mejiaj
@thisisdano thisisdano Awaiting requested review from thisisdano
@amyleadem amyleadem Awaiting requested review from amyleadem

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aduth @mejiaj