Skip to content

USWDS - Header: Alignment fixes #5649

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 6, 2023
Merged

USWDS - Header: Alignment fixes #5649

merged 7 commits into from
Dec 6, 2023

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Nov 30, 2023 •
edited
Loading

Summary

Updated the padding on .usa-nav__submenu elements so that they align with other header elements.

Breaking change

This is not a breaking change.
However, we recommend that users confirm that the header elements in their project align as expected.

Related issue

Closes #5417
Closes #5604

Related pull requests

This PR is a combination of PRs #5418 and #5630.

Changelog PR

Preview link

Problem statement

Header navigation links do not align with other header elements. Standard variants align too far to the left and megamenu variants align too far to the right.

From PR #5418:

USWDS 2 had some padding in nav submenus so that the submenu items were visually aligned with the nav menu button itself (see https://github.com/uswds/uswds/blob/v2.13.3/src/stylesheets/components/_navigation.scss#L374). This padding was removed in USWDS 3, and the nav submenus look off.

Solution

Testing and review

  • Open each variant of the header component and confirm:
    • All navigation links align to the left of the other header elements.
    • Megamenu link elements also align to the right of other header elements.
    • Mobile presentation is not affected.
    • No visual regressions

@amyleadem amyleadem added this to the uswds 3.7.1 milestone Nov 30, 2023
@amyleadem
Copy link
Contributor Author

@mejiaj and @mahoneycm Here are a couple of things to consider in your review.

  • I made an update to standardize right alignment on the extended megamenu variant in 0d65c1c. The content extended beyond the width of the navbar, and it would be great to get your confirmation that this was the correct choice.
  • There is now increased left padding on link elements in the megamenu variants. This makes the link content a bit narrower, which makes me a bit concerned about increased risk of new line breaks (though this will also be true of the change in 0d65c1c). Curious to hear your thoughts on this.

@amyleadem amyleadem marked this pull request as ready for review November 30, 2023 17:20
@amyleadem amyleadem removed this from the uswds 3.7.1 milestone Nov 30, 2023
@mejiaj
Copy link
Contributor

mejiaj commented Dec 1, 2023

@amyleadem

Right alignment

  • I made an update to standardize right alignment on the extended megamenu variant in 0d65c1c. The content extended beyond the width of the navbar, and it would be great to get your confirmation that this was the correct choice.

Makes sense. I tried to keep the original PR as simple as possible to avoid regressions and to speed up review/testing. Thanks for making this fix.

Narrow links in Megamenu

  • There is now increased left padding on link elements in the megamenu variants. This makes the link content a bit narrower, which makes me a bit concerned about increased risk of new line breaks (though this will also be true of the change in 0d65c1c). Curious to hear your thoughts on this.

How much narrower? Is this a big impact? Otherwise makes sense to continue.

@amyleadem
Copy link
Contributor Author

@mejiaj The link elements are now about 16px narrower than before. I included some screenshots in the description to show the change. It should be easiest to see in the screenshot for Header component - megamenu. I believe this is happening because the updated link padding is being universally applied to all variants.

We could either narrow how that padding is being applied or add an override to the megamenu links. I'd expect it to be a quick one-line fix to revert the megamenu link styles.

My preference to make the change to preserve the link width in megamenu variants to match the narrower padding in develop. Curious to hear what you think.

@amyleadem
Copy link
Contributor Author

@mejiaj @mahoneycm I added a padding override for megamenu submenu links in 7222ba5 and I updated the related screenshots in the description. Let me know what you think!

mahoneycm
mahoneycm approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Lgtm! Thanks Amy!

  • Standard dropdown links properly aligned to their parent dropdown text
  • Megamenu links do not surpass search bar
  • Megamenu maintains expected line breaks
  • Mobile presentation unaffected
  • No unintentional visual regressions

@amyleadem amyleadem mentioned this pull request Dec 4, 2023
Merged
mejiaj
mejiaj approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

I tested mobile and desktop, all variants. Both left/right alignment looks good.

@amyleadem amyleadem linked an issue Dec 6, 2023 that may be closed by this pull request
USWDS - Bug: Adjust alignment in headers with megamenu #5604
Closed
@amyleadem amyleadem requested a review from thisisdano December 6, 2023 15:56
echappen added a commit to 18F/guides that referenced this pull request Dec 6, 2023
@echappen
override USWDS extended header alignment issue
13300a3 
Overrides USWDS extended header submenu. This is fixed in uswds/uswds#5649 and will be released in v3.7.1. In the meantime, we'll keep this override.
thisisdano
thisisdano approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Nice

@thisisdano thisisdano merged commit 91e99e4 into develop Dec 6, 2023
@thisisdano thisisdano deleted the header-nav-link-alignment branch December 6, 2023 18:19
@amyleadem amyleadem mentioned this pull request Dec 6, 2023
Closed
@thisisdano thisisdano mentioned this pull request Dec 8, 2023
Merged
@SherfeyInv SherfeyInv mentioned this pull request Jun 7, 2024
Closed
@sennhann29 sennhann29 mentioned this pull request Jul 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@mejiaj mejiaj mejiaj approved these changes

@thisisdano thisisdano thisisdano approved these changes

@mahoneycm mahoneycm mahoneycm approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
USWDS Core Project Data
Archived in project
Milestone
No milestone
5 participants
@amyleadem @mejiaj @thisisdano @mahoneycm @lpsinger