Skip to content
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

Fix mobile primary navigation styles #225

Merged
merged 1 commit into from
Jun 17, 2021
Merged

Fix mobile primary navigation styles #225

merged 1 commit into from
Jun 17, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 16, 2021

Why: The USWDS upgrade in #219 changed default padding for most navigation menus. This was corrected in the side navigation, but mobile primary navigation still appeared more condensed than previously. Additionally, default scaling for the navigation close button was changed in USWDS to be much larger due to a change to use the new icon set, causing the icon to appear very large. The design system was not using our custom version of the icon anyways. This has been corrected.

Before After
image image

Open questions:

  • Is this what we want the menu to look like? I couldn't find a canonical reference for what the mobile menu should look like. The adjustments here are a best compromise of restoring previous expectations. This could be a good opportunity to make updates if we wanted.
  • Do we want to use the USWDS icon? Expanding on the above note about USWDS transitioning to new icon set for the mobile "Close" button, it could help simplify our overrides if we use the USWDS icon. That said, while it is very similar in appearance to our custom icon, it's not exactly the same; specifically, there's a slight rounding to the linecaps in our version.

**Why**: The USWDS upgrade in #219 changed default padding for most navigation menus. This was corrected in the side navigation, but mobile primary navigation still appeared more condensed than previously. Additionally, default scaling for the navigation close button was changed in USWDS to be much larger due to a change to use the new icon set, causing the icon to appear very large. The design system was not using our custom version of the icon anyways. This has been corrected.
Comment on lines -124 to -126
&:hover {
color: color('primary-dark');
}
Copy link
Member Author

@aduth aduth Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -30 to -31
padding-top: units(1.5);
padding-bottom: units(1.5);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again not relevant for primary navigation, but in trying to find a canonical reference for the menu, I noticed a small difference in our sublist styling from the Figma reference. Previously, sublist items had smaller padding, but in the Figma design they're identical padding.

BeforeAfter
beforeafter

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aduth
Copy link
Member Author

aduth commented Jun 17, 2021

Visual regression appears to be attributable to the changes described at #225 (comment).

@aduth aduth merged commit 2515e41 into main Jun 17, 2021
@aduth aduth deleted the aduth-fix-navbar-close branch June 17, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants