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

SubdomainNavBar overflow fix #451

Merged
merged 53 commits into from
Oct 24, 2023

Conversation

joseph-lozano
Copy link
Contributor

@joseph-lozano joseph-lozano commented Oct 5, 2023

Summary

This PR fixes 2 issues:

  1. If the sandwich menu was open on narrow screen sizes, and then the viewport was widened, nav items would be duplicated.
  2. If there were too many nav items for the height of the viewport, they would overflow into the CTA actions. ([Accessibility] Improve overflowing of SubdomainNavBar on mobile #405)

Fixes #405

List of notable changes:

The if the Nav menu is open, the background will no longer be scrollable, but the Nav menu will be.

What should reviewers focus on?

Verify that the SubdomainNavBar is working as intended, and there were no unintended side-effect changes.

Steps to test:

  1. Go to MobileMenuOpenManyItems in Storybook and verify that it is working as intended.
  2. Verify that it is working as intended.
  3. Ensure all other stories are unchanged

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before After

CleanShot 2023-10-05 at 12 50 22@2x

CleanShot 2023-10-05 at 12 50 42@2x

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

🦋 Changeset detected

Latest commit: e5fdb21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

🟢 No design token changes found

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@joseph-lozano joseph-lozano temporarily deployed to github-pages October 5, 2023 17:10 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano force-pushed the joseph-lozano/SubdomainNavbar-overflow-fix branch from ce55f07 to e488e8a Compare October 5, 2023 17:11
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 5, 2023 17:22 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 6, 2023 13:04 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 6, 2023 14:12 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano force-pushed the joseph-lozano/SubdomainNavbar-overflow-fix branch from 6a4976a to f1955b5 Compare October 6, 2023 14:16
Joseph Lozano added 2 commits October 23, 2023 09:36
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 23, 2023 13:49 — with GitHub Actions Inactive
Joseph Lozano added 2 commits October 23, 2023 10:01
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 24, 2023 13:49 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 24, 2023 14:04 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano temporarily deployed to github-pages October 24, 2023 14:20 — with GitHub Actions Inactive
@joseph-lozano joseph-lozano marked this pull request as ready for review October 24, 2023 14:38
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nsolerieu @natalie-iv - FYI due to space constraints, we're proposing to drop the title at the narrowest viewport 👇

Screenshot 2023-10-24 at 17 34 38

It will reappear again at the next larger breakpoint.

Is this cool with y'all? FYI, we also already have an option to use this pattern without the title, even at desktop, so this is building on top of that precedent.

Copy link
Contributor

@nsolerieu nsolerieu left a comment

Choose a reason for hiding this comment

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

Make sense, shit it 🚀

I think it makes sense considering space/size but we are loosing an important semantic element which my create a bit of confusion... but I don't have a (great) solution to offer.

Copy link
Collaborator

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

LGTM...

In the words of @nsolerieu, shit it :shipit: 🤣

Screenshot 2023-10-24 at 18 09 02

@joseph-lozano joseph-lozano merged commit b15ab24 into main Oct 24, 2023
14 checks passed
@joseph-lozano joseph-lozano deleted the joseph-lozano/SubdomainNavbar-overflow-fix branch October 24, 2023 18:05
@primer-css primer-css mentioned this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Accessibility] Improve overflowing of SubdomainNavBar on mobile
3 participants