-
Notifications
You must be signed in to change notification settings - Fork 33
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
SubNav v2 #824
SubNav v2 #824
Conversation
🦋 Changeset detectedLatest commit: 4a89735 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
🔍 Design token changes foundView CSS variable changes- --brand-SubNav-color-link-active: var(--base-color-scale-blue-5);
+ --brand-SubNav-color-link-active: var(--brand-color-text-default); - --brand-SubNav-color-link-active: var(--base-color-scale-blue-3);
+ --brand-SubNav-color-link-active: var(--brand-color-text-default); - --brand-SubNav-color-subMenu-bgColor: var(--base-color-scale-black-0);
+ --brand-SubNav-color-subMenu-bgColor: var(--base-color-scale-white-0); |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
...on-SubNav-Mobile-viewport-test-e0f45-en-SubNav-Narrow-Dropdown-Variant-Menu-Open-1-linux.png
Outdated
Show resolved
Hide resolved
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.
Commenting to remove Changes requested status
nvm, that didn't work 😶
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.
Looking great!
This looks really great! Just a few minor visual adjustments are needed based on my design review:
AnchorNav top and bottom paddings should be set to Dropdown items should align with the parent link above. Link to the Figma implementation for reference. I'll preemptively approve so you can merge when these tweaks are ready. |
Summary
Part of https://github.com/github/primer/issues/3312
Updates
SubNav
component to support anchor-based navigation similar toAnchorNav
.SubNav v2
there are no breaking changes to the API as part of this update. A full visual refresh however, has been implemented per latest Site guidance.🔗 Documentation
🔗 Storybook examples
List of notable changes:
SubNav.Submenu
to supportvariant="anchor"
What should reviewers focus on?
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots:
New anchor navigation