USWDS: Banner - Remove grid-layout dependency. Resolves #4868

[mahoneycm]

Component

Preview →

Description

This PR removes the grid-layout dependency to cut down on the component's package size. It uses flex to maintain the same look as before.

Resolves #4868

How to test

Isolate Banner package

• In uswds/_index.scss remove all packages besides usa-banner
• Remove /src/styles so it only reads @forward "usa-banner"

Check output css file size

• Run npx gulp buildSass to compile CSS
• Check dist/css/uswds.css filesize (I use a vscode plugin but you can cd the directory to it and run open .
• With this change it should be around 17kb. Before it was about 47kb.

If you do the above steps on another branch, you should see the larger filesize.

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.

[mejiaj]

@mahoneycm thanks for taking this issue on. I can see layout grid dependency was removed and tested the output. I've noticed on larger screens (approximately 1200px and higher) there's a visual discrepancy to the current banner.

cm-usa-banner-package (top) vs develop (bottom)

Banner on develop stays centered on larger screens and the one in this PR stays to the left. Mobile seems to look good. Can you look into this issue and then check the compiled CSS file size again?

[mahoneycm]

@mejiaj Looks like grid-container was setting a max width and auto margins to center it. I added some utility mixins to fix it!

[mejiaj]

Banner (mobile/desktop) looks good! I also compiled using only the banner package and can confirm, no utility layout classes.

[amyleadem]

@mahoneycm
I tried compiling only the banner package by calling only usa-banner and usa-media-block in uswds/index.scss. When I do this, I see that the content gets stacked rather than sitting side by side. Could this code be missing the rules from @include grid-row?

[amyleadem]

@mahoneycm
I turned off all packages except for usa-banner and noticed some visual discrepancies between this and the current banner display (only tested in Chrome so far)

Let me know if you have questions.

[mahoneycm]

Resolved above comment!

[mahoneycm]

Thanks to @amyleadem's recommendation above, I realized I overworked my solution and we are able to continue to use the grid mixins. Cleaned up my solution to include only what was necessary.

New changes made

[amyleadem]

Looks good to me! Here is how I tested:

1. Turned off all Sass except for uswds-core, uswds-fonts, usa-media-block, and usa-banner
2. Checked the appearance in Firefox, Safari, Chrome, and Microsoft for Edge

Visuals matched what I saw on develop in the these browsers and the code looks nice and clean. CSS output is 18kb.

Note: I noticed that there are some grid classes inside usa-banner__header that aren't addressed in this PR, but the visual presentation still matched develop when the layout-grid package was turned off. It seems to be fine, but wanted to flag it in case it has a side effect I am not seeing.

[mejiaj]

Looks good, I was able to test all variants of banner on both mobile and desktop sizes. Just a comment on the specificity of the grid selector.

Compile test
To test compile I did the following:

1. Went to packages/uswds/_index.scss and commented everything except banner
2. ⚠️ Change @forward "usa-banner/src/styles"; → @forward "usa-banner";. I wonder if the /src/styles is intentional or if we should only point to the package.
3. Compiled and verified only usa-banner styles
4. Ran npm start and did a visual check of all banners

[mahoneycm]

@thisisdano Ready for final review!

[thisisdano]

Perfect. Really happy to see this change!