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

LG-4226: Fix double focus ring on unstyled buttons #200

Merged
merged 4 commits into from
Mar 10, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 10, 2021

Regression of #193 (69a13a0).

Why: For consistency with other buttons and to align to expected design specs, there should only be a single outline on any focussed button.

Before After
before after

Live Preview: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-fix-unstyled-focus/components/buttons/

Implementation Notes:

Refactoring in #193 rearranged styles such that our re-application of the unstyled mixin would reintroduce focus styles through the mixin's use of link styles. Ultimately, including this mixin should not be necessary, because USWDS handles hover and active unstyled states, but does not currently do so for the modifier classes (see uswds/uswds#4077). Previously, we worked around this by including the unstyled button mixin, which would have a higher specificity than the default button's hover styles. Unfortunately, this also comes with some unexpected consequences, such as the re-application of the USWDS focus ring, which we expect to override in our own default button styles.

This branch includes a bump to the patch version of the package, with the expectation that a new package will be published immediately following merge.

**Why**: It should not be necessary, because USWDS handles hover and active unstyled states, but does not currently do so for the modifier classes. Previously, we worked around this by including the unstyled button mixin, which would have a higher specificity than the default button's hover styles. Unfortunately, this also comes with some unexpected consequences, such as the re-application of the USWDS focus ring, which we expect to override in our own default button styles.
Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Focus ring looks good! Looks like the large unstyled buttons now have left/right padding though, while the regular size unstyled buttons don't. I would vote to remove the padding here and not specify it at the design system level for either size of unstyled buttons (see screenshot in #193 ), but lmk if you feel otherwise

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

…rrides

**Why**: We customize big button padding and border radius, which should not extend to apply to the unstyled button. This is controlled in USWDS by applying unstyled styles last. We must similarly reapply unstyled button styles after our customizations. Unlike prior to afecbdd, don't give extra specificity and instead rely on style order, similar to as in base USWDS styles.
@aduth
Copy link
Member Author

aduth commented Mar 10, 2021

Looks like the large unstyled buttons now have left/right padding though, while the regular size unstyled buttons don't.

Good catch! That definitely wasn't intended. As I detailed in the extended commit description of e55f619, this has to do with how we're customizing the big button padding. The fix is to restore part of the previous implementation, to make sure we get the padding reset (and all other "base" unstyled styles) we expect.

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

LGTM!

@aduth aduth merged commit 2994661 into main Mar 10, 2021
@aduth aduth deleted the aduth-fix-unstyled-focus branch March 10, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants