USWDS - Megamenu: Fix error with $theme-header-max-width: "none" #5072

amyleadem · 2022-12-16T16:19:00Z

Summary

Added a none token 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-width is set to none.

Breaking change

This is not a breaking change.

Related issue

Closes #4851

Problem statement

Setting $theme-header-max-width: "none", throws the following error npx gulp compile:

node_modules/@uswds/uswds/packages/uswds-core/src/styles/functions/units/units.scss
Error: ("1px": 1px, "2px": 2px, "05": 0.25rem, 1: 0.5rem, "105": 0.75rem, 2: 1rem, "205": 1.25rem, 3: 1.5rem, "neg-1px": -1px, "neg-2px": -2px, "neg-05": -0.25rem, "neg-1": -0.5rem, "neg-105": -0.75rem, "neg-2": -1rem, "neg-205": -1.25rem, "neg-3": -1.5rem, 4: 2rem, 5: 2.5rem, 6: 3rem, 7: 3.5rem, 8: 4rem, 9: 4.5rem, 10: 5rem, 15: 7.5rem, "neg-4": -2rem, "neg-5": -2.5rem, "neg-6": -3rem, "neg-7": -3.5rem, "neg-8": -4rem, "neg-9": -4.5rem, "neg-10": -5rem, "neg-15": -7.5rem, "card": 10rem, "card-lg": 15rem, "mobile": 20rem, "mobile-lg": 30rem, "tablet": 40rem, "tablet-lg": 55rem, "desktop": 64rem, "desktop-lg": 75rem, "widescreen": 87.5rem, 0: 0, "auto": auto) isn't a valid CSS value.
   ╷
28 │       vars.$project-spacing-standard
   │       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  node_modules/@uswds/uswds/packages/uswds-core/src/styles/functions/units/units.scss 28:7    units()
  node_modules/@uswds/uswds/packages/usa-header/src/styles/_usa-megamenu.scss 15:10           outer-megamenu()
  node_modules/@uswds/uswds/packages/usa-header/src/styles/_usa-megamenu.scss 52:7            @content
  node_modules/@uswds/uswds/packages/uswds-core/src/styles/mixins/helpers/at-media.scss 20:7  at-media()
  node_modules/@uswds/uswds/packages/usa-header/src/styles/_usa-megamenu.scss 51:5            @forward
  node_modules/@uswds/uswds/packages/usa-header/src/styles/_index.scss 5:1                    @forward
  sass/_uswds-packages.scss 27:1                                                              @import
  sass/uswds.scss 6:9                                                                         root stylesheet

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-width through units(), which only recognizes spacing tokens. Because none is 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:

$theme-header-max-width
$theme-input-max-width
$theme-in-page-nav-main-content-max-width
$theme-modal-default-max-width
$theme-modal-lg-max-width

Solution

By adding a none token, we can allow users to set any theme max-width setting to none.

Additionally, adding a conditional to the outer-megamenu mixin in usa-header.scss allows the system to provide the correct value and prevent error when the value is "none".

Testing and review

Confirm that setting $theme-header-max-width: "none" does not cause an error. Also confirm that a "none value works with the other max-width settings.
Confirm that a value of "none" creates the expected visual with various values for $theme-header-max-width, including none.
Confirm that other spacing token values still work in $theme-header-max-width

…header-max-width

mejiaj

@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: (), 
        
           ),

packages/usa-header/src/styles/_usa-megamenu.scss

This reverts commit 8778d33.

amyleadem · 2023-01-31T18:11:08Z

📓 Note: PR #4757 and related issue #4234 explain why we care about calculating the exact width of the pseudo elements.

amyleadem · 2023-01-31T23:06:55Z

@mejiaj I feel like I found a cleaner solution. Let me know what you think! 786895a

mejiaj

@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!

mejiaj · 2023-02-01T16:46:38Z

packages/usa-header/src/styles/_usa-megamenu.scss

+    $mw: 100vw;
+  }
+
+  // subtract half the value of the viewport width from half the value of the submenu width


// 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?

mejiaj · 2023-02-01T16:51:39Z

packages/usa-header/src/styles/_usa-megamenu.scss

+  }
+
+  // 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


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:

Suggested change

// 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.

amyleadem · 2023-02-01T17:42:49Z

@mejiaj Thanks for poking at this to get a better solution!
I cleaned up the comments to be shorter and more accurate. Let me know if you see anything that needs adjustment.

mejiaj

Looks great, thank you!

thisisdano

Nice one!

Amy Leadem added 3 commits December 16, 2022 09:09

Add 'none' spacing token

8c531d2

Revert spacing none

36c3de4

Add conditional for -max-width == 'none'

3850be4

amyleadem changed the title ~~USWDS - Header: Add 'none' spacing token~~ USWDS - Header: Fix error with $theme-header-max-width: "none" Dec 16, 2022

amyleadem changed the title ~~USWDS - Header: Fix error with $theme-header-max-width: "none"~~ USWDS - Megamenu: Fix error with $theme-header-max-width: "none" Dec 16, 2022

Amy Leadem added 5 commits December 16, 2022 14:16

Add conditional for max-width=none and standard/extended

c40929b

Undo commented line

e86325c

Add 'none' spacing token

1fd220e

Add comments

49170c3

Merge branch 'develop' of https://github.com/uswds/uswds into al-fix-…

98f8a1e

…header-max-width

amyleadem marked this pull request as ready for review December 20, 2022 21:00

amyleadem requested a review from mejiaj December 20, 2022 21:00

amyleadem modified the milestone: uswds 3.4.0 Dec 21, 2022

This comment was marked as outdated.

Sign in to view

mejiaj reviewed Jan 25, 2023

View reviewed changes

packages/usa-header/src/styles/_usa-megamenu.scss Outdated Show resolved Hide resolved

Amy Leadem added 3 commits January 26, 2023 12:03

Simplify outer-megamenu conditional

8778d33

Revert "Simplify outer-megamenu conditional"

2492e62

This reverts commit 8778d33.

Add outer megamenu width calc conditionals for when max-width:none

c3d08f8

amyleadem requested a review from mejiaj January 26, 2023 23:20

Set when header-max width is none

786895a

mejiaj reviewed Feb 1, 2023

View reviewed changes

Amy Leadem added 2 commits February 1, 2023 10:36

Clean up outer-megamenu comments

4e97c31

Fix typos

afda7db

amyleadem requested a review from mejiaj February 1, 2023 17:42

mejiaj approved these changes Feb 1, 2023

View reviewed changes

mejiaj requested a review from thisisdano February 1, 2023 19:36

thisisdano approved these changes Feb 28, 2023

View reviewed changes

thisisdano merged commit 53ded2f into develop Feb 28, 2023

thisisdano deleted the al-fix-header-max-width branch February 28, 2023 19:05

thisisdano mentioned this pull request Mar 9, 2023

USWDS 3.4.0 #5170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

USWDS - Megamenu: Fix error with $theme-header-max-width: "none" #5072

USWDS - Megamenu: Fix error with $theme-header-max-width: "none" #5072

Uh oh!

amyleadem commented Dec 16, 2022 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

mejiaj left a comment •

edited

Loading

Uh oh!

Uh oh!

amyleadem commented Jan 31, 2023

Uh oh!

amyleadem commented Jan 31, 2023

Uh oh!

mejiaj left a comment

Uh oh!

mejiaj Feb 1, 2023

Uh oh!

mejiaj Feb 1, 2023

Uh oh!

amyleadem commented Feb 1, 2023

Uh oh!

mejiaj left a comment

Uh oh!

thisisdano left a comment

Uh oh!

Uh oh!

	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: (),
	),

	// 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.

USWDS - Megamenu: Fix error with $theme-header-max-width: "none" #5072

USWDS - Megamenu: Fix error with $theme-header-max-width: "none" #5072

Uh oh!

Conversation

amyleadem commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking change

Related issue

Problem statement

Solution

Testing and review

Uh oh!

This comment was marked as outdated.

Uh oh!

mejiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amyleadem commented Jan 31, 2023

Uh oh!

amyleadem commented Jan 31, 2023

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

mejiaj Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem commented Feb 1, 2023

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

thisisdano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amyleadem commented Dec 16, 2022 •

edited

Loading

mejiaj left a comment •

edited

Loading