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 - Footer: Remove grid dependency. #5289
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danbrady I appreciate the considerations.
Existing footer styles has reference to "disclosure button styles". I didn't remove them, but don't see them in any template markup. Are these needed?
Do you mean this comment on disclosure links?
uswds/packages/usa-footer/src/styles/_usa-footer.scss
Lines 103 to 145 in cf8c634
// Disclosure button functionality happens at mobile widths | |
&--button { | |
@include place-icon($-chevron-expand-more, "before", 0.5); | |
width: 100%; | |
border: 0; | |
cursor: pointer; | |
&:not([disabled]):focus { | |
// Apply negative focus offset to ensure that entire focus ring is visible | |
@include focus-outline(null, null, null, "neg-05"); | |
} | |
// Arrow for collapsible content. | |
&::before { | |
@include u-square(2.5); | |
align-items: center; | |
background-size: contain; | |
content: ""; | |
display: inline-flex; | |
justify-content: center; | |
margin-right: units(0.5); | |
margin-left: units(-0.5); | |
@media (forced-colors: active) { | |
background-color: buttonText !important; | |
} | |
} | |
& + .usa-list--unstyled { | |
margin-top: units(1); | |
margin-bottom: units(1); | |
} | |
&[aria-expanded="false"] { | |
@include place-icon($-chevron-next, "before", 0.5); | |
// Hide submenu when button is not expanded | |
& + .usa-list--unstyled { | |
display: none; | |
} | |
} | |
} | |
} |
Yes, they’re needed. They’re visible in Big Footer variants collapsible links ― only on mobile.
They’re dynamically generated using JS:
https://github.com/danbrady/uswds/blob/develop/packages/usa-footer/src/index.js#L33-L74
I updated existing styles to use utility mixins as opposed to native CSS properties. It was mixed before, is there a preference?
Yes, the main preference is to use native with design tokens like color()
, units()
, etc. We tend to use mixins for shorthands like u-padding-y
or u-margin-x
.
As mentioned in the possible limitations above, I only brought in the columns that are used for the footer variations. If this is the right approach, I can make a mixin to output these in a more concise, readable way.
We should take an approach similar to Banner - Remove grid-layout dependency. Where usa-layout-grid
was removed, but we still kept the classes. Renaming classes right now would negatively impact users. Especially those on small teams that maintain multiple sites.
I make use of the 'gap' property for flexbox items. This is not supported in IE11 but all other supported browsers. USWDS v3 states IE11 support was dropped. Is this ok?
Yes, that’s fine. Thanks for double-checking.
FYI: I also separated the logo lockup and address into includes as the markup was the same for multiple footer variations.
Thank you! Improving templates is something we’d like to do, but difficult to prioritize over other work.
e1a61bf
to
f937a43
Compare
Thanks for the feedback, @mejiaj. I appreciate you taking the time to answer (especially as this PR has gotten bigger!). I believe I incorporated everything correctly. Please review the updated PR as your time permits. A few heads-up things:
That's all for now. Let me know of any next steps. Thanks for letting me geek out on this! 🤓 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far, I've added some notes/suggestions for improvements. I did notice there's a visual change in the logo + agency name. The logo & name are no longer vertically centered.
On the bright side, the logo in slim variant looks a lot better.
Also, if you hit the re-review
button I'm less likely to miss updates. Otherwise, direct pings work ― just might be slower.
packages/usa-footer/src/_includes/usa-footer-nav-primary--big.twig
Outdated
Show resolved
Hide resolved
Thanks for the thorough feedback, @mejiaj. My mistake, I didn't realize we should not change the markup at all and should solely rely on CSS for these changes. That would avoid a breaking change. I'll go back and revise the PR. And sorry for the other (embarrassing) silly mistakes... I'll review better before re-requesting a review. |
Happy to give feedback and I appreciate all the work you've done on this. Also, apologies for the miscommunication on markup changes. As reviewers we have to be as thorough as possible, but it can be tough! With design systems or libraries that many teams depend on the stakes are higher. We have to try to minimize negative impact as much as possible. |
f937a43
to
2d6f240
Compare
Ok, updated the PR to revert any markup changes and only made updates to the CSS. I think that handles the majority of the issues discovered. Regarding the grid classes brought in, if I found a specific |
Markup changes were reverted in #5289 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I didn't find any bugs, but noting some visual differences in review comments. Going to mark this with the discussion tag so we can review as a team.
I've also pushed up a branch (db-remove-grid-from-footer
) to get preview links generated.
Variant | Branch links | Develop links |
---|---|---|
Footer (default) | preview | develop |
Footer big | preview | develop |
Footer slim | preview | develop |
Table updated 10/02/23, based on comit 3619d87
Discussed proposed changes with the team. Marking this PR as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there Dan,
Comparing this PR to #4868 where we removed the grid dependency from Banner. It looks like the code you added between lines 31-123 might be all we need to replicate the grid layout, without the style changes on the lines to follow!
Did you experience any visual bugs after simply redefining the grid-
classes that led you to update the footer classes further? In my testing, I didn't come across any further visual discrepancies.
Thank you,
Charlie
Hi @mahoneycm! Thanks for the thorough review. You are right that bringing in just the grid classes would satisfy losing the grid dependency. However, there are other visual quirks that I noticed while developing that needed style adjustments (and without touching markup). One issue was identified in #5214. Other issues are with the slim version in mobile view having some quirkiness (a screenshot is here: #5289 (comment)), wrapped social icons don't have same vertical space between them as horizontal space, and there a few other minor tweaks. This PR fixes all of these. Truthfully, I intended to just swap the grid classes and fix the space on the right, but as I was working on them, I noticed other things. If it makes more sense to split this up into different issues/PRs so it's smaller changes, I can do that. Let me know! Thanks! |
@danbrady Hey there Dan! After some discussion with the team, we'd like to separate these changes into two separate PRs which will merge into an integration branch before PRing against develop. This will allow us to:
Are you able to separate the work and create an integration branch to PR each against? We'll prioritize testing these changes since you are going above and beyond to work with us on this update! We really appreciate it. If you have any questions just let me know! |
Hey @mahoneycm! Absolutely. I can see how this PR has way too many changes and can difficult to review, so splitting up makes total sense. Apologies if I impacted the the team's time. So just to be clear, I'll create a new integration branch in my repo then new separate branches for 1) solely removing grid dependency for #5287 and 2) fixing the space/alignment issue for #5214. Then I'll submit those PRs individually for review. For any other issue that I uncovered (like the visual quirks in mobile slim footer), I'll create a new issue (if it doesn't exist) and separate PR. That's what I should have done when I noticed them, but hey, lesson well learned. 🤓 |
@danbrady you're a rockstar! 👨🎤 I really appreciate your willingness to work within our structure! Our processes have been improving and reviewing them this way will enable us to get your changes implemented quicker. Thanks Dan! |
2d6f240
to
3619d87
Compare
No problem, @mahoneycm! I've worked on many projects without a well-defined structure, so I rather appreciate the standardize processes your team has in place! Thanks for bearing with me getting up to speed on them. Ok, I did some git-fu and broke down this PR. This should solely remove the grid dependencies now. Ready for your review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working great for me! Thanks for all the effort that went into this!
- No visual regression across variants or screensizes
- No build errors
- sassTests pass
- Ran prettier && sass linting
- Installed on USWDS-site with no visual regression
- Code follows USWDS standards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working with us on this one. I tested the following:
- No visual regressions on small, medium, large screen sizes.
- Grid layout dependency has been removed.
- Zero regressions when only importing
@forward "usa-footer";
.
I also measured the file sizes and found the following:
File | Before (compile) | After |
---|---|---|
uswds.css |
661 KB | 622 KB |
uswds.min.css |
525 KB | 491 KB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
Summary
Removed grid dependency from Footer package. Now you no longer need to use the usa-layout-grid package with usa-footer resulting in a much smaller package size.
Important
Layout changes
If you notice changes in your layout due to this change, add the
usa-layout-grid
package to your Sass entry point.If this does not resolve all changes, set
$theme-layout-grid-use-important: true
in your theme settings.Breaking change
This is potentially a breaking change.
Users who are not importing
usa-layout-grid
but have adjusted the footer markup may see layout changes.Related issue
Closes #5287
Related pull requests
TBD
Note: Remove layout-grid from the dependency lists from doc site.
Problem statement
The footer currently has the layout-grid dependency although very little of it is used. This adds additional, unnecessary file weight.
Solution
Removed the grid layout dependency and brought layout styles into the footer directly.
Possible limitation: I only brought in the column sizes that are needed to support the three variations of the footer (default, slim, big) at all the breakpoints. If alternate layouts are desired, addition styles would need to be added. For example, changing the number of columns or how wide the link columns span in the big footer.
Testing and review
Existing unit test still completes successfully.
Compared the unminified CSS size of the Footer before and after changes: 63kb -> ~40kb
Isolate Footer package
uswds/_index.scss
remove all packages besidesusa-banner
dist/css/uswds.css
filesizeLooking for feedback, please:Existing footer styles has reference to "disclosure button styles". I didn't remove them, but don't see them in any template markup. Are these needed?I updated existing styles to use utility mixins as opposed to native CSS properties. It was mixed before, is there a preference?As mentioned in the possible limitations above, I only brought in the columns that are used for the footer variations. If this is the right approach, I can make a mixin to output these in a more concise, readable way.I make use of the 'gap' property for flexbox items. This is not supported in IE11 but all other supported browsers. USWDS v3 states IE11 support was dropped. Is this ok?FYI: I also separated the logo lockup and address into includes as the markup was the same for multiple footer variations.FYI: @mejiaj