USWDS: Banner - Remove grid-layout dependency. Resolves #4868#5000
Conversation
There was a problem hiding this comment.
@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?
|
@mejiaj Looks like |
mejiaj
left a comment
There was a problem hiding this comment.
Banner (mobile/desktop) looks good! I also compiled using only the banner package and can confirm, no utility layout classes.
There was a problem hiding this comment.
@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
left a comment
There was a problem hiding this comment.
@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.
|
Resolved above comment! |
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
|
Thanks to @amyleadem's recommendation above, I realized I overworked my solution and we are able to continue to use the |
There was a problem hiding this comment.
Looks good to me! Here is how I tested:
- Turned off all Sass except for
uswds-core,uswds-fonts,usa-media-block, andusa-banner - 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
left a comment
There was a problem hiding this comment.
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:
- Went to
packages/uswds/_index.scssand commented everything except banner ⚠️ Change@forward "usa-banner/src/styles"; → @forward "usa-banner";. I wonder if the/src/stylesis intentional or if we should only point to the package.- Compiled and verified only
usa-bannerstyles - Ran
npm startand did a visual check of all banners
|
@thisisdano Ready for final review! |
thisisdano
left a comment
There was a problem hiding this comment.
Perfect. Really happy to see this change!
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
uswds/_index.scssremove all packages besidesusa-banner/src/stylesso it only reads@forward "usa-banner"Check output css file size
npx gulp buildSassto compile CSSdist/css/uswds.cssfilesize (I use a vscode plugin but you can cd the directory to it and runopen .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:
npm testand make sure the tests for the files you have changed have passed.