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

USWDS - Normalize: Remove outdated normalizations #5555

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Oct 12, 2023

Summary

Update normalize styles to remove outdated normalizations. Reduces the compiled size of normalization styles.

Related issue

Closes #4701

Problem statement

As a developer, I expect that USWDS applies its stated browser support to source code, so that I'm not misled by unexpected behaviors, or unnecessarily inflating the size of my compiled artifacts for end-users.

As an end-user, I expect that sites using USWDS load quickly, so that I'm not stuck waiting for pages to load in order to access the content I'm seeking.

Solution

The solution here refines the existing normalize stylesheet to prune styles which are not needed, either due to improvements in the affected browsers, or due to the browser no longer being supported (e.g. Internet Explorer). These refinements were applied manually, using the following projects as references:

An alternative / future-proof solution could consider incorporating something like PostCSS Normalize, which aims to automatically prune styles from the base normalization script using a project's own browserslist configuration.

@mejiaj mejiaj added the Status: Triage We're triaging this issue and grooming if necessary label Oct 12, 2023
@thisisdano thisisdano added Role: Dev Development/engineering skills needed Type: Enhancement An improvement to existing code and removed Status: Triage We're triaging this issue and grooming if necessary labels Nov 2, 2023
@thisisdano thisisdano added this to the uswds 3.8.0 milestone Nov 2, 2023
@mejiaj mejiaj modified the milestones: uswds 3.8.0, uswds 3.7.1 Nov 22, 2023
@amyleadem amyleadem modified the milestones: uswds 3.7.1, uswds 3.8.0 Nov 30, 2023
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

LGTM — thanks for adding the license info!

Browsers tested

  • Chromium 120
  • Firefox 122
  • Safari 17.1.2

I also created a test branch [test-aduth-modern-normalize] on uswds/uswds-site and found no issues in Accordion or Banner (tested because they rely on hidden).

This update saves 2KB on CSS, 6KB → 4KB.

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

This looks good to me! Just one question left on a changed line.

Note

I had to pull the latest changes from develop to resolve some visual discrepancies that have been previously addressed.

  • No build errors
  • No visual regressions perceived on changed elements
  • Visual display consistent across browsers

packages/uswds-elements/lib/_normalize.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

I tested this in Firefox, Safari, and Chromium and didn't see any regressions for the changed elements.

I wanted to flag that I found one small difference in appearance in <hr> elements in Firefox after this change, but I don't think it should be considered a blocker. I've added details in the comment below.

*/

hr {
box-sizing: content-box; /* 1 */
height: 0; /* 1 */
overflow: visible; /* 2 */
color: inherit; /* 2 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Note

This changes the color of the <hr/> in firefox. After this change, the color now matches the value set for $theme-text-color. It is a subtle difference that can be hard to capture in the screenshot, but can be found in its computer color value:

image

I am assuming that this is desired behavior, but wanted to flag this as a change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe this is expected and was able to trace this back to smoothing out a Firefox inconsistency.

See:

@mejiaj
Copy link
Contributor

mejiaj commented Mar 8, 2024

Really appreciate all the work done here.

We've put a hold on this PR for now, but we're going to look at how we can include this and other opt out features in an upcoming release.

For now, I've moved it to the 3.9.0 release milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
From: GSA Role: Dev Development/engineering skills needed Type: Enhancement An improvement to existing code
Projects
Status: Hold
Development

Successfully merging this pull request may close these issues.

Replace normalize with a more modern CSS reset
5 participants