USWDS - Header: Use $theme-header-min-width for safari fix calc#5624
USWDS - Header: Use $theme-header-min-width for safari fix calc#5624thisisdano merged 7 commits intodevelopfrom
Conversation
mahoneycm
left a comment
There was a problem hiding this comment.
Great catch! Looks like this slipped through since theme-header-max-width and theme-header-min-width both shared the same value.
- No visual regression
- Fix is logical based on the origin of the bug
- Safari bug fix works as expected
- Calc error is no longer present
mejiaj
left a comment
There was a problem hiding this comment.
I still see invalid CSS and an error on latest from al-safari-bug-fix-regression.
Steps to reproduce
- Set
$theme-header-min-width: "none" - Run
npx gulp buildSass - Confirm error at the end.
[11:43:19] 'compileSass' errored after 4.29 s
[11:43:19] CssSyntaxError in plugin "gulp-postcss"
Message:
postcss-csso: /Users/jmejia-a/web/uswds/dist/css/uswds.css:9423:13: Number, dimension, ratio or identifier is expected
9421 | }
9422 |
> 9423 | @media (min-width: calc(none - 0.94rem)){
| ^
9424 | .usa-js-mobile-nav--active.is-safari{
9425 | overflow-y:scroll;
[11:43:19] 'buildSass' errored after 9.76 s
thisisdano
left a comment
There was a problem hiding this comment.
Now that I'm digging in, I still see the error that James mentions
|
Ok, I'll fix it up and will tag you both for re-review when it's ready. |
|
Sorry I jumped into this too soon! |
mahoneycm
left a comment
There was a problem hiding this comment.
This is looking good to me! One comment about special characters but expected values look great!
- Confirm that there is no compilation error when you set
$theme-header-max-widthto "none" - Confirm that the Safari bug fix from USWDS - Nav: Fix compilation error when max header width is set to "none" #5617 still works by checking if the mobile menu opens as expected in Safari:
- Confirm that the mobile menu opens as expected in other browsers:
- Confirm that the bug fix works with different values for
$theme-header-min-width - Confirm that we don't need to account for any special values for
$theme-header-min-widththat could cause an error (like "none")
| position: fixed; | ||
| // --scrolltop set with JS with zero as fallback. | ||
| top: var(--scrolltop, 0); | ||
| @if $theme-header-min-width != "none" { |
There was a problem hiding this comment.
Special Tokens
I'm getting a build error when setting $theme-header-min-width: "auto"
Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
Undefined operation "calc(auto - 0.94rem) > 0".
╷
487 │ @if $safari-header-bug-min-width > 0 {
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
╵
packages/usa-nav/src/styles/_usa-nav.scss 487:7 @forward
packages/usa-nav/src/styles/_index.scss 4:1 @forward
packages/uswds/_index.scss 38:1 root stylesheet- This is a regression from
develop. - But it should be noted that setting
autobreaks the layout of the header. - Would the error be preferred? If so, should we update the acceptable tokens to exclude values that break the layout? (negative values, 0,)
- Potentially out of scope
Important
I began experiencing inconsistent builds and build errors during my testing. There is a chance my broken layouts for certain special tokens might have been a local issue.
There was a problem hiding this comment.
@mahoneycm I updated the Safari fix to only run if the value of $theme-header-min-width fits a realistic value for the setting.
The header visually breaks and causes warnings when the value of $theme-header-min-width is outside of the defined breakpoints in our system (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen). This is because the setting is used primary with our at-media mixin, which only accepts the breakpoints defined in $system-properties.
For this reason, I decided to bypass the Safari fix unless the setting was defined with the same values that are acceptable to at-media.
Warning
There is some risk with this approach. If we ever update the accepted values for the at-media mixin to be something other than the system breakpoints, the Safari fix might not be applied in all the right places. However, I feel like the breakpoints in $system-properties will be a stable and reliable place to look for realistic header breakpoints. In my mind, this reduces the risk of future issues.
More on the thought process
- First I tried bypassing only the "special" values of “auto” and “none”. This worked in most cases but would cause an error with 1px/2px values because of the unit mismatch.
- This caused me to think about how it would be better to only apply the safari fix to *realistic* values, rather than *possible* values. The header visually breaks and causes warnings when the value of `$theme-header-min-width` is outside of the defined breakpoints in our system (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen). For this reason, I decided to bypass the Safari fix unless the setting was defined with one of those values.
- Alternatively, we could create a custom map that includes all possible reasonable values, which could mean including the "medium" spacing token set. I ultimately moved away from this because it seemed very rare that a mobile nav would be viewed in safari <120px, especially if the header is broken in other ways by this value.
Can you confirm the following?
- Setting
$theme-header-min-widthto any breakpoint value (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen)- DOES generate an
is-safariclass in the CSS - does NOT cause a compile error
- DOES generate an
- Setting
$theme-header-min-widthto any non-breakpoint value (Ex: "none", "auto", 15, "hamburger")- does NOT generate an
is-safariclass in the CSS - does NOT cause a compile error. However, this warning is still expected for non-breakpoint values:
- does NOT generate an
Warning: `none` is not a valid USWDS project breakpoint. Valid values: card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen
There was a problem hiding this comment.
I'm still seeing is-safari. The logic looks good and the build warning are appearing, but the safari class is added. I've tried rebuilding the project multiple times.
The confusing thing is when I run a debug, it returns false as expected and should not be running the block of scss 🤔
@debug map-has-key($our-breakpoints, $theme-header-min-width); // Output: falseUpdate: I misunderstood and was looking for the markup change... CSS looks good 😎
- Standard breakpoint generates
is-safariclass - No compilation errors
- Non-breakpoint values bypass safari fix in compiled CSS
- Non-breakpoint values cause compilation warning, but no error
There was a problem hiding this comment.
Note
Updated the changelog PR to reflect changes to this PR. Feel free to adjust further!
summary: Resolved a compilation error when $theme-max-header-width is set to non-breakpoint values.
|
@mejiaj This is ready for review. You can find details about the most recent update in this comment as part of a previous discussion. Curious to hear what you think! |
mejiaj
left a comment
There was a problem hiding this comment.
Nice fix. I tested:
$theme-header-min-width: "none";$theme-header-min-width: "tablet-lg";$theme-header-min-width: "desktop"
No errors or invalid CSS compiled.
Summary
Fixed a bug that prevented
$theme-max-header-widthfrom accepting a value of "none".Breaking change
This is not a breaking change.
Related issue
Closes #5579
Related pull requests
This PR replaces the fix proposed in #5617.
Changelog PR
Preview link
Preview link: Documentation page
Problem statement
A compilation error occurs when setting
$theme-max-header-widthto"none".This is a regression which stems from our Safari header navigation bug fix in #5543. The calc() method cannot take a value of none so it breaks the calculation.
Solution
The calculation mistakenly used
$theme-header-max-widthinstead of$theme-header-min-width. The settings have two distinct purposes:$theme-header-min-widthis the setting that determines where the mobile nav should change to the desktop nav.$theme-header-max-widthis the setting that determines the maximum width of the desktop navigation.For the Safari fix to work as expected, the fix needs to be triggered within 15px of the mobile/desktop breakpoint, which is
$theme-header-min-width.By removing
$theme-header-max-widthfrom the calculation, there should no longer be errors when the setting's value is set to "none".Testing and review
$theme-header-max-widthto "none"$theme-header-min-width$theme-header-min-widthto expected values like "widescreen" or "tablet"$theme-header-min-width$theme-header-min-widththat could cause an error (like "none")