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

Buttons: Improve unstyled button styles to visually match link appearance #201

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 22, 2021

Related Slack conversation: https://gsa-tts.slack.com/archives/CNCGEHG1G/p1616421634001700

Why: The use of an unstyled button is intended to take the visual appearance of a link. The proposed changes resolve some discrepancies that currently exist:

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

Screenshot:

Before After
before after

This was originally observed in the IdP, where an equivalent link and unstyled button did not appear the same. See examples below which show a real link on the left vs. an unstyled button on the right. The IdP customizes the default font smoothing of text, which currently causes a difference in appearance between links and unstyled buttons (see uswds/uswds#4115). Separately, we should consider if this is actually something we should be doing, perhaps as part of the ongoing work to migrate to Public Sans font (see related guidance).

IdP Before IdP with underline offset fix IdP with underline offset & smoothing fixes
idp before idp with underline fix idp with underline and smoothing fix

@aduth
Copy link
Member Author

aduth commented Mar 22, 2021

Interesting that the tests pass 🤔 I would expect visual regressions to be flagged here.

@zachmargolis
Copy link
Contributor

Interesting that the tests pass 🤔 I would expect visual regressions to be flagged here.

I tried running locally, got some timeouts, but also this:

 FAIL  test/screenshot.test.js (118.968 s)
  ● screenshot visual regression

    assert.strictEqual(received, expected)

    Expected value to strictly be equal to:
      0
    Received:
      4
    
    Message:
      Expected "/research/1-flow-diagrams/" to visually match the live site.

🤷

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

@aduth
Copy link
Member Author

aduth commented Mar 22, 2021

but also this:

The diffs should be in your local relative tmp folder to take a look at what it might be considering different. 4 seems like a strange result, since I'm pretty sure that number is "total pixel difference", and 4 feels very small. If I had to guess looking at that particular page, I'd guess something with how those very-large-images are loaded or scaled.

I recall some recent discussion about possibly removing / moving / updating those pages, which might incidentally help clear up some of the flakiness around that page.

@aduth
Copy link
Member Author

aduth commented Mar 22, 2021

Interesting that the tests pass 🤔 I would expect visual regressions to be flagged here.

Taking a look locally, I think the issue is that text-underline-offset support in Chrome is relatively new (v87), and the version of Chromium used in the visual regression tests is a bit older, and therefore the previous styling effectively did nothing because it wasn't supported.

@aduth aduth merged commit c0b788a into main Mar 23, 2021
@aduth aduth deleted the aduth-unstyled-buttons-links branch March 23, 2021 13:39
Comment on lines +33 to +34
-moz-osx-font-smoothing: inherit;
-webkit-font-smoothing: inherit;
Copy link
Member Author

Choose a reason for hiding this comment

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

These styles only apply for hover, active, and disabled 🤦 Should have also duplicated above, after the @include button-unstyled. Was misled by the fact that this is substituting the behavior of no-knockout-font-smoothing.

aduth added a commit that referenced this pull request Mar 23, 2021
**Why**: These were applied to modifier states for unstyled buttons in #201, but should have also been applied to the base styles, to override USWDS unstyled button styles.
aduth added a commit that referenced this pull request Mar 23, 2021
**Why**: These were applied to modifier states for unstyled buttons in #201, but should have also been applied to the base styles, to override USWDS unstyled button styles.
aduth added a commit to 18F/identity-idp that referenced this pull request Mar 24, 2021
**Why**: 18F/identity-design-system#202 and 18F/identity-design-system#201 intended to inherit font smoothing, but unfortunately miss these two states which are handled and inherited from USWDS.
aduth added a commit to 18F/identity-idp that referenced this pull request Mar 24, 2021
* Upgrade to identity-style-guide 5.0.3

**Why**: Fix unstyled button link appearance

* LG-3865: Update BassCSS link buttons to USWDS

**Why**: We should be able to expect that the visual appearance of BassCSS `btn-link` and the USWDS unstyled button are identical. As such, migrating to the unstyled button can happen independently of the broader BassCSS migration to simplify future effort.

Excludes SimpleForm buttons, since all SimpleForm buttons will apply the `btn btn-primary` class which may conflict with USWDS buttons. This is also why the class isn't yet deprecated in the erb-lint.yml file.

See: https://github.com/18F/identity-idp/blob/ce7608a50ea9f7b8d8c747c63cc0c49549eac4a7/config/initializers/simple_form.rb#L5

* Revert hover color to default link color

See inline comment

* Update selector for back link test

* Preserve link color for visited, unstyled buttons

**Why**: See inline code comment

* Apply inherited font smoothing to hover, active unstyled buttons

**Why**: 18F/identity-design-system#202 and 18F/identity-design-system#201 intended to inherit font smoothing, but unfortunately miss these two states which are handled and inherited from USWDS.
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.

3 participants