-
Notifications
You must be signed in to change notification settings - Fork 934
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 - Megamenu: Fix error with $theme-header-max-width: "none" #5072
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.
@amyleadem added a suggestion that isn't too far away from the original intent and it only checks for standard values once. I'd like to avoid relying on multiple @if statements to try to keep things as straightforward as possible.
I couldn't figure out why max-width() values error when the standard property has map.get($partial-values, "none"),
uswds/packages/uswds-core/src/styles/_properties.scss
Lines 476 to 488 in 98f8a1e
| max-width: ( | |
| standard: | |
| map-collect( | |
| map.get($system-spacing, "small"), | |
| map.get($system-spacing, "medium"), | |
| map.get($system-spacing, "large"), | |
| map.get($system-spacing, "larger"), | |
| map.get($system-spacing, "largest"), | |
| map.get($partial-values, "none"), | |
| map.get($partial-values, "full-percent") | |
| ), | |
| extended: (), | |
| ), |
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.
@amyleadem love the simpler solution! I've tested visual display in Chrome, Firefox, and Safari with no issues found.
I have a few questions on the comments, but the solution looks good!
| $mw: 100vw; | ||
| } | ||
|
|
||
| // subtract half the value of the viewport width from half the value of the submenu width |
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.
// subtract half the value of the viewport width from half the value of the submenu width
Does this comment apply to the entire statement or a specific line? Can we also simplify or shorten this comment?
| } | ||
|
|
||
| // subtract half the value of the viewport width from half the value of the submenu width | ||
| // standard needs the additional $theme-site-margins-width to accommodate different paddings/structure on #basic-mega-nav-section-two |
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.
Does this comment make sense here or should we move it inside?
Let's break up long comments to next line to keep these comments readable. In my editor they look like this:
| // standard needs the additional $theme-site-margins-width to accommodate different paddings/structure on #basic-mega-nav-section-two | |
| // Standard needs the additional $theme-site-margins-width to | |
| // accommodate different paddings/structure on Megamenu variant. |
|
@mejiaj Thanks for poking at this to get a better solution! |
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 great, thank you!
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 one!

Summary
Added a
nonetoken to the system spacing tokens. This allows users to set a value of"none"for any max-width setting, including$theme-header-max-width.Additionally, this PR updated the
outer-megamenu()mixin to prevent error when$theme-header-max-widthis set tonone.Breaking change
This is not a breaking change.
Related issue
Closes #4851
Problem statement
Setting
$theme-header-max-width: "none",throws the following errornpx gulp compile:This happens because the width of the megamenu pseudo-elements is determined with a calculation that includes
$theme-header-max-width. This calculation runs$theme-header-max-widththroughunits(), which only recognizes spacing tokens. Becausenoneis not a spacing token, it throws an error.The following settings also run their values through
units(), and a value of"none"results in the same error:Solution
By adding a
nonetoken, we can allow users to set any thememax-widthsetting tonone.Additionally, adding a conditional to the
outer-megamenumixin inusa-header.scssallows the system to provide the correct value and prevent error when the value is"none".Testing and review
$theme-header-max-width: "none"does not cause an error. Also confirm that a"nonevalue works with the othermax-widthsettings."none"creates the expected visual with various values for$theme-header-max-width, includingnone.$theme-header-max-width