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-3187: Upgrade USWDS to 2.9.0 #159

Merged
merged 18 commits into from Nov 30, 2020
Merged

LG-3187: Upgrade USWDS to 2.9.0 #159

merged 18 commits into from Nov 30, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 13, 2020

Related (merges to): #158

Preview on Federalist: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-uswds-upgrade/

This pull request upgrades USWDS dependency from ^2.0.3. to ^2.9.0 (latest).

Notes:

  • See additional documented findings in ticket LG-3187 description
  • There have already been a few conflicts in the use of colors. As noted in the research findings, USWDS v2.4.0 introduced additional support for non-token values. The USWDS internal implementation makes some assumptions about the readiness of tokens at initialization, which conflicted with previous usage of token assignment to theme variables. Furthermore, there's at least one upstream bug in USWDS bypassing color tokens which does not interoperate with custom theme colors (source, pending upstream bugfix).

@aduth
Copy link
Member Author

aduth commented Oct 13, 2020

Furthermore, there's at least one upstream bug in USWDS bypassing color tokens which does not interoperate with custom theme colors (source, pending upstream bugfix).

See uswds/uswds#3762

Base automatically changed from aduth-visual-regression to master October 14, 2020 16:56
@aduth
Copy link
Member Author

aduth commented Oct 14, 2020

Regression: Banner link reverts to blue, resulting in accessibility failures.

Screenshot (before):

image

Error:

    "http://0.0.0.0:35819/accessibility/policies/": [
      {
        "code": "WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail",
        "context": "<span class=\"usa-banner__button-text\">Here’s how you know</span>",
        "message": "This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 2.66:1. Recommendation: change text colour to #fdfeff.",
        "type": "error",
        "typeCode": 1,
        "selector": "html > body > div:nth-child(2) > div > header > div > button > span"
      }
    ],

Fix: 23f63c9

Impact on other products: None, assuming upgrade, since changes are exclusive to stylesheet updates.

Screenshot (after):

image

@aduth
Copy link
Member Author

aduth commented Oct 14, 2020

Regression: USWDS upgraded the major version of Normalize.css, causing certain opinionated styling to be omitted. This causes pre tags to unexpectedly overflow.

Related: https://github.com/necolas/normalize.css/blob/master/CHANGELOG.md#400-march-19-2016

Screenshot (before):

image

Fix: b6dc182, e50689e

Impact on other products: None, assuming upgrade, since changes are exclusive to stylesheet updates.

Screenshot (after):

image

@aduth
Copy link
Member Author

aduth commented Oct 14, 2020

Update from research findings:

Header and banner width now controlled in settings: Now there are explicit settings for controlling the max-width of the gov banner and the header.

Affected variables are renamed in 9fcc61c.

See: https://designsystem.digital.gov/about/releases/#version-220

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

Acknowledged and expected change: Footer text changes from Serif (Merriweather) to Sans-Serif (Source Sans Pro).

Screenshot:

Before After
before after

Related changelog: USWDS v2.3.0

Now components reliably respect their font settings. Setting values like $theme-footer-font-family should set the font face for the entire component, but some CSS specificity quirks were overriding these values in some instances. Now, setting a component’s font face works as expected, with no secret overrides. (uswds/uswds#3253)

Relevant variable assignment:

https://github.com/18F/identity-style-guide/blob/b196ccfcfc3da550275e43eb21e021f6581e2c83/src/scss/uswds-theme/_components.scss#L36

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

Acknowledged and expected change: Line-heights on most content has changed slightly, in direct correlation with a fix in USWDS 2.2.0 to improve rounding precision of the line-height functions. This is only explicitly mentioned to affect navigation, but the affected functions (px-to-rem, line-height, lh) are used extensively throughout USWDS. The effect is that pages may extend longer vertically than they had previously (verified on documentation site with visual regression test results).

Pull request: uswds/uswds#3100

Fixes a spacing issue affecting the header nav indicator bars present since 2.0 that seems to be a result of rounding errors in line-height calculation.

Because this had a significant impact on visual regression testing, I was able to assess the impact of these and other changes by effectively reverting the fix: https://gist.github.com/aduth/77c4d3785666088282cee22f37acd981

The impact is noticeable only under scrutinous review of differences.

Example:

Before After
before after

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

⚠️ Requires markup changes at time of upgrade in dependent projects ⚠️

USWDS 2.5.0 includes updates to markup for the benefit of improved accessibility.

banner: is now a <section> instead of a <div> with the ARIA label Official government website
footer: nav includes the ARIA label of Footer navigation
graphic-list: uses h2 as a heading default instead of h3
hero: includes the ARIA label of Introduction
search: the search form is given the ARIA role of search
documentation template: includes only the main content in the <main> element. The nav is no longer treated as an <aside>

https://designsystem.digital.gov/about/releases/#version-250

Examples updated in: 64f32de

This will require corresponding markup changes in consuming projects at time of upgrade.

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

⚠️ Requires markup changes at time of upgrade in dependent projects ⚠️

USWDS 2.6.0 updates markup of search component.

Use more consistent search markup. We made the markup of usa-search more consistent and assured that the styling worked properly regardless of what version of the markup you’re using. This is not as breaking change, but we recommend updating your search markup to the most current version. (uswds/uswds#3327)

https://designsystem.digital.gov/about/releases/#version-260

Examples updated in: a611187

This will require corresponding markup changes in consuming projects at time of upgrade.

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

⚠️ Requires markup changes at time of upgrade in dependent projects ⚠️

USWDS 2.7.0 updates markup guidance for numeric input fields:

Improved mobile experience for numeric fields. We updated our guidance and code for numeric fields to follow the lead of gov.uk’s recent guidance and research. This updates those fields to use text rather than number inputs with an inputmode of numeric. (uswds/uswds#3392)

https://designsystem.digital.gov/about/releases/#version-270

Examples updated in: 231ebd3

This will require corresponding markup changes in consuming projects at time of upgrade.

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

Acknowledged and expected change: USWDS 2.7.0 fixes appearance of skip link to appear as link. This is desirable, no changes are necessary, and consuming projects will benefit immediately by upgrading.

Before After
before after

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

⚠️ Requires markup changes at time of upgrade in dependent projects ⚠️

USWDS 2.7.1 updates markup of SVG images to include role attribute.

Added role="img" to SVG images. (uswds/uswds#3501)

https://designsystem.digital.gov/about/releases/#version-271

Examples updated in: a67fff0

This will require corresponding markup changes in consuming projects at time of upgrade. As a general accessibility best-practice, it could be implemented independently from the LGDS upgrade in consuming projects.

@aduth
Copy link
Member Author

aduth commented Oct 15, 2020

Marking this as ready for review, since all necessary changes from upgrades should be accounted for.

Preview on Federalist: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-uswds-upgrade/

I'm planning to include a CHANGELOG.md file to help document some of the above-noted required changes for consuming projects.

Also noting: The build failure for ci/circleci: visual-regression is expected, as described in above "Acknowledged and expected change" notes and by visual regression tests documentation.

@aduth aduth marked this pull request as ready for review October 15, 2020 14:56
@aduth
Copy link
Member Author

aduth commented Oct 16, 2020

⚠️ Requires markup changes at time of upgrade in dependent projects ⚠️

USWDS 2.8.0 updates markup of the "official government website" banner.

https://designsystem.digital.gov/about/releases/#version-280

Updated in documentation site in: 029b089

This will require corresponding markup changes in consuming projects at time of upgrade.

@aduth aduth added the needs: ready for review This pull request is ready to be looked at by any team member (especially those tagged as reviewers) label Oct 19, 2020
@its-a-lisa-at-work its-a-lisa-at-work added this to Reviewed Required in TTS Tech Portfolio (Pull Requests) via automation Oct 22, 2020
@its-a-lisa-at-work its-a-lisa-at-work removed this from Reviewed Required in TTS Tech Portfolio (Pull Requests) Oct 23, 2020
@juliaelman juliaelman self-requested a review October 29, 2020 14:36
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, looks like there is a visual regression in the accordion but it looks like mostly padding and I'm OK with it if you are

$theme-color-disabled: 'site-disabled-grey';
$theme-color-disabled-dark: 'site-darker-grey';
// TODO: Revert back to `map-get` once https://github.com/uswds/uswds/pull/3762 is released.
$theme-color-disabled-light: 'gray-5'; // map-get($site-palette, 'site-lightest-grey');
Copy link
Contributor

Choose a reason for hiding this comment

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

should we standardize on gray vs grey? (looking at $theme-color-disabled-family above). My inclination is to use gray because that's standard for US English?

Copy link
Member Author

Choose a reason for hiding this comment

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

should we standardize on gray vs grey? (looking at $theme-color-disabled-family above). My inclination is to use gray because that's standard for US English?

tl;dr: I think so, yes.

It might be worth getting broader UX feedback here, but it kinda seems like the whole set of $theme-color-grey variables should be eliminated altogether. At least comparing to the base USWDS theme color tokens, I don't see any reference to where grey originates from. Grays appear to fit within the "base" family, and we only reference grey under the base colors in our own color guide.

It may make sense to keep our own $site-palette, but considering that USWDS spells it as gray in their own system tokens, I agree it'd make sense to align it.

In the short-term though, I'm a bit wary to include it here, since it has some cascading effects, but in terms of making breaking changes in one pass, it could be wise to do before publishing a version of this package that contains USWDS 2.9.0.

@aduth
Copy link
Member Author

aduth commented Nov 17, 2020

visual regression in the accordion but it looks like mostly padding and I'm OK with it if you are

Yeah, the prior comments worked to mitigate most of the major differences, but the remaining diff is attributed to a new line height calculation that's expected (#159 (comment)).

@aduth
Copy link
Member Author

aduth commented Nov 17, 2020

Couple more findings that will need reconciliation:

@aduth
Copy link
Member Author

aduth commented Nov 17, 2020

  • USWDS 2.8.0 includes a new Tooltip component, which conflicts with our custom component (live site, branch preview, USWDS component)

    • Suggestion: Remove our custom implementation, defer to USWDS, include note in CHANGELOG describing markup differences for upgrade.

As far as impact is concerned, a naive search appears to reveal that we don't use it anywhere:

https://github.com/search?l=&q=usa-tooltip+user%3A18f&type=code

I can still document the change all the same.

**Why**: Clear up console warnings on build
@aduth
Copy link
Member Author

aduth commented Nov 27, 2020

Chatted with @nickttng on Slack regarding the notes in #159 (comment):

  • For Date Picker and Date Range Picker components, we're in agreement to consider this if and when they become relevant for us to use and customize.
  • For Tooltip, given the fact that we don't currently use it anywhere, it seems reasonable to remove this for now to allow us to move forward with a release. If we discover a need for tooltips in the future, we can investigate how best to reconcile our guidance with USWDS'.

@aduth
Copy link
Member Author

aduth commented Nov 27, 2020

Another discovery I'd made is that USWDS introduces a Card component in Version 2.7.0. While there are some visual similarities to the LGDS Card component, the usage guidance is not quite the same, where the USWDS discourages standalone usage of a card.

While it's an unfortunate naming collision, the implementation of the LGDS card using custom lg--based classes means there should not be concern for conflicts in code. Thus, we can upgrade to the latest USWDS without concern for unexpected breakage. Technically, there's nothing to stop a developer from using both or either of lg-card and usa-card.

Recommendation: No action for now. In the future, consider how to reconcile where reconciliation could be one of (a) removing our card implementation in favor of USWDS, (b) renaming our card component if it should be distinct from USWDS card component, or (c) merge implementation and document guidance for different usage if desired to build upon USWDS card.

Note that future revisions or removal of this component should consider impact to dependent applications, where it is currently used in 18f/identity-dashboard and a test route in identity-idp (see search results).

Copy link
Member

@juliaelman juliaelman left a comment

Choose a reason for hiding this comment

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

Excellent work @aduth! 🚀

@aduth aduth merged commit 64b6f69 into master Nov 30, 2020
@aduth aduth deleted the aduth-uswds-upgrade branch November 30, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: ready for review This pull request is ready to be looked at by any team member (especially those tagged as reviewers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants