Conversation
🦋 Changeset detectedLatest commit: aa41671 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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-bgColor: var(--brand-color-canvas-default);- --brand-SubNav-color-subMenu-bgColor: var(--base-color-scale-white-0);
+ --brand-SubNav-color-link-bgColor: #e4ebe6;+ --brand-SubNav-bgColor: var(--brand-color-canvas-default);- --brand-SubNav-color-subMenu-bgColor: var(--base-color-scale-white-0);
+ --brand-SubNav-color-link-bgColor: #323834; |
🟢 Unit test coverage changes foundUnit test coverage has been updated through this PR. Changes: 0 new tests, 0 removed tests, 1 improved, 1 decreased
|
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
There was a problem hiding this comment.
Pull request overview
Refactors the SubNav component to match updated Brand styles (including borders/gridlines by default), introduces new SubNav design tokens, and updates tests/docs/snapshots. It also propagates aria-current support through the FlexTemplate recipe so “current page” state can be reflected in SubNav links.
Changes:
- Refactor
SubNavmarkup/styling (icons, active states, narrow overlay behavior) and deprecatehasShadow. - Add
aria-currentsupport toFlexTemplatelink types + stories and pass it through toSubNav.Link. - Update visual testing (snapshots, flaky-story skipping) and docs/changesets/tokens for the new SubNav look.
Reviewed changes
Copilot reviewed 13 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/recipes/FlexTemplate/examples/FlexTemplate.examples.stories.tsx | Marks “current” FlexTemplate links in examples via aria-current. |
| packages/react/src/recipes/FlexTemplate/FlexTemplate.types.ts | Extends link type to support 'aria-current'. |
| packages/react/src/recipes/FlexTemplate/FlexTemplate.tsx | Passes aria-current through to SubNav.Link. |
| packages/react/src/SubNav/SubNav.tsx | Core SubNav refactor: icons, narrow toggle structure/ARIA, overlay sizing, active state class. |
| packages/react/src/SubNav/SubNav.module.css | New SubNav visuals (borders, typography, pill active styles, overlay layout tweaks). |
| packages/react/src/SubNav/SubNav.module.css.d.ts | CSS module typings updated for new class names. |
| packages/react/src/SubNav/SubNav.test.tsx | Updates unit tests to reflect new behavior (separator test removed, new accessible-name expectations). |
| packages/react/src/SubNav/SubNav.visual.spec.ts | Removes flaky “Anchor Nav Variant” visual test while keeping narrow/other scenarios. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Sub-Heading-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Full-Width-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Forwarded-Refs-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Default-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Anchor-Nav-Default-Link-Variant-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-SubNav-Active-Sub-Heading-1-linux.png | Updated visual snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-for-Sub-Heading-Narrow-SubNav-Sub-Heading-Narrow-1-linux.png | Updated mobile snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-for-Full-Width-Narrow-SubNav-Full-Width-Narrow-1-linux.png | Updated mobile snapshot for new styling. |
| packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-0855d-ing-Narrow-SubNav-Active-Sub-Heading-Narrow-1-linux.png | Updated mobile snapshot for new styling. |
| packages/e2e/scripts/playwright/update-visual-snapshots | Allows passing extra Playwright args through the snapshot update script. |
| packages/e2e/scripts/playwright/playwright.generate-tests.ts | Skips the flaky anchor-nav variant story in generated visual tests. |
| packages/design-tokens/src/tokens/functional/components/sub-nav/colors.js | Adds/adjusts SubNav tokens (background + link bgColor). |
| apps/next-docs/content/components/SubNav/index.mdx | Removes hasShadow docs/example and updates props table. |
| .changeset/proud-pans-cross.md | Changeset for new SubNav tokens in @primer/brand-primitives. |
| .changeset/icy-cities-swim.md | Changeset documenting SubNav refactor + hasShadow deprecation + Action variant change. |
danielguillan
left a comment
There was a problem hiding this comment.
Great updates! They work perfectly.
| value: 'var(--base-color-scale-white-0)', | ||
| dark: '#000', |
There was a problem hiding this comment.
We should be able to use --(--brand-color-cavas-default) here, which maps to white-0 (#fff) and black-0 (#000) and is semantically appropriate.
There was a problem hiding this comment.
perfect, thanks Dani. I assumed that we don't use #000 exactly for black-0, but that's only in light mode. Now both use canvas.
| font-size: calc(var(--base-size-16) - 1px); | ||
| line-height: calc(var(--base-size-16) - 1px); |
There was a problem hiding this comment.
🙈 Is that 1px difference significant enough to warrant deviating from the type scale?
There was a problem hiding this comment.
I know 😅. This matches the design exactly. I normally would push for standardisation but 16px IMO did feel a bit too big, and we don't have anything lower until 12px. As it's the SubNav I think it's okay as a one-off, but let me add an annotation to make it clear it's an exception. Does that work for you @danielguillan?
Summary
Towards https://github.com/github/brand-marketing-design/issues/2606
Updatest the visual appearance of SubNav and deprecates one prop.
🔗 Docs
🔗 Storyboook
List of notable changes:
hasShadowpropWhat should reviewers focus on?
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots: