Skip to content

Commit c7d36b9

Browse files
rezrahrezrah
authored
Fix separator rendering bug in SubNav (#1120)
1 parent 907d661 commit c7d36b9

19 files changed

+199
-23
lines changed

‎.changeset/poor-balloons-jam.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@primer/react-brand': patch
3+
---
4+
5+
Fix layout shift in `SubNav` by ensuring separator visibility is determined pre-hydration.

‎packages/react/src/SubNav/SubNav.features.stories.tsx‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -508,3 +508,49 @@ AnchorNavVariantKeyboardNavigation.play = async ({canvasElement}) => {
508508
await userEvent.tab({shift: true})
509509
expect(getByRole('link', {name: 'Premium Support'})).toHaveFocus()
510510
}
511+
512+
export const NoActiveLinks = args => (
513+
<main>
514+
<Box paddingBlockStart={64} backgroundColor="subtle" style={{position: 'relative', zIndex: 32}}></Box>
515+
<SubNav {...args}>
516+
<SubNav.Heading href="#">Features</SubNav.Heading>
517+
<SubNav.Link href="#">Actions</SubNav.Link>
518+
<SubNav.Link href="#">Packages</SubNav.Link>
519+
<SubNav.Link href="#">Security</SubNav.Link>
520+
<SubNav.Link href="#">Codespaces</SubNav.Link>
521+
<SubNav.Link href="#">Copilot</SubNav.Link>
522+
<SubNav.Link href="#">Code review</SubNav.Link>
523+
<SubNav.Link href="#">Search</SubNav.Link>
524+
<SubNav.Link href="#">Issues</SubNav.Link>
525+
<SubNav.Link href="#">Discussions</SubNav.Link>
526+
<SubNav.Action href="#">Get started</SubNav.Action>
527+
</SubNav>
528+
<Grid>
529+
<Grid.Column>
530+
<Hero align="center">
531+
<Hero.Label>GitHub Features</Hero.Label>
532+
<Hero.Heading>Choose the tools that work best for your team.</Hero.Heading>
533+
<Hero.Description>
534+
This story demonstrates the SubNav component when no links have aria-current=&quot;page&quot; set, which
535+
should result in no separator being visible according to the test expectations.
536+
</Hero.Description>
537+
<Hero.PrimaryAction href="#">Explore features</Hero.PrimaryAction>
538+
</Hero>
539+
</Grid.Column>
540+
</Grid>
541+
</main>
542+
)
543+
NoActiveLinks.parameters = {
544+
layout: 'fullscreen',
545+
}
546+
NoActiveLinks.storyName = 'With no aria-current set'
547+
548+
export const NoActiveLinksNarrow = () => <NoActiveLinks />
549+
NoActiveLinksNarrow.parameters = {
550+
layout: 'fullscreen',
551+
viewport: {
552+
defaultViewport: 'iphonex',
553+
},
554+
}
555+
556+
NoActiveLinksNarrow.storyName = 'With no aria-current set (narrow)'

‎packages/react/src/SubNav/SubNav.module.css‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,12 @@
8282

8383
.SubNav__heading-separator {
8484
position: relative;
85-
top: var(--base-size-2);
85+
top: 0;
8686
color: var(--brand-color-text-muted);
87+
min-width: var(--base-size-8);
88+
min-height: var(--base-size-16);
89+
display: inline-flex;
90+
align-items: center;
8791
}
8892

8993
/*
@@ -275,6 +279,12 @@
275279
animation: fade-in 0.3s var(--brand-animation-easing-glide) forwards;
276280
}
277281

282+
.SubNav__heading-separator--main:not(.SubNav__heading-separator--has-adjacent-label),
283+
.SubNav__heading-separator--subheading-active,
284+
.SubNav__subheading-container-active {
285+
display: none;
286+
}
287+
278288
.SubNav__header-container {
279289
display: flex;
280290
width: 100%;
@@ -286,10 +296,6 @@
286296
z-index: 9998;
287297
}
288298

289-
.SubNav__heading-separator:not(.SubNav__heading-separator--has-adjacent-label) {
290-
display: none;
291-
}
292-
293299
.SubNav__links-overlay {
294300
position: relative;
295301
display: flex;

‎packages/react/src/SubNav/SubNav.module.css.d.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ declare const styles: {
1919
readonly "SubNav__heading-label": string;
2020
readonly "SubNav__heading-separator": string;
2121
readonly "SubNav__heading-separator--has-adjacent-label": string;
22+
readonly "SubNav__heading-separator--main": string;
23+
readonly "SubNav__heading-separator--subheading-active": string;
2224
readonly "SubNav__link": string;
2325
readonly "SubNav__link--expanded": string;
2426
readonly "SubNav__link--has-sub-menu": string;
@@ -38,6 +40,7 @@ declare const styles: {
3840
readonly "SubNav__sub-menu-list": string;
3941
readonly "SubNav__sub-menu-toggle": string;
4042
readonly "SubNav__subHeading": string;
43+
readonly "SubNav__subheading-container-active": string;
4144
readonly "fade-in": string;
4245
readonly "fade-in-down": string;
4346
};

‎packages/react/src/SubNav/SubNav.test.tsx‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -174,19 +174,6 @@ describe('SubNav', () => {
174174
expect(separator).toBeInTheDocument()
175175
})
176176

177-
it('does not render a separator when there are no links with `aria-current="page"` set', () => {
178-
const {queryByRole} = render(
179-
<MockSubNavFixture
180-
data={[
181-
{title: 'page one', href: '#page1'},
182-
{title: 'page two', href: '#page2'},
183-
]}
184-
/>,
185-
)
186-
187-
expect(queryByRole('separator', {hidden: true})).not.toBeInTheDocument()
188-
})
189-
190177
it('shows the aria-current text next to the button by default', () => {
191178
const {getByRole} = render(<MockSubNavFixture />)
192179

‎packages/react/src/SubNav/SubNav.tsx‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,17 @@ type SeparatorProps = {
124124
activeLinklabel?: string
125125
} & BaseProps<HTMLSpanElement>
126126

127-
function Separator({activeLinklabel}: SeparatorProps) {
127+
function Separator({activeLinklabel, className, ...props}: SeparatorProps) {
128128
return (
129129
<span
130130
role="separator"
131131
className={clsx(
132132
styles['SubNav__heading-separator'],
133133
activeLinklabel && styles['SubNav__heading-separator--has-adjacent-label'],
134+
className,
134135
)}
135136
aria-hidden
137+
{...props}
136138
>
137139
<svg xmlns="http://www.w3.org/2000/svg" width="8" height="16" viewBox="0 0 8 16" fill="none" aria-hidden>
138140
<g clipPath="url(#clip0_50_1307)">
@@ -323,14 +325,29 @@ const _SubNavRoot = memo(
323325
<div ref={innerRootRef} className={styles['SubNav--header-container-outer']}>
324326
<div className={styles['SubNav__header-container']}>
325327
{HeadingChild && <div className={styles['SubNav__heading-container']}>{HeadingChild}</div>}
326-
{SubHeadingChild && (isLarge || !subHeadingIsActive) && (
328+
329+
{SubHeadingChild && (
327330
<>
328-
<Separator activeLinklabel={activeLinklabel} />
329-
<div className={styles['SubNav__heading-container']}>{SubHeadingChild}</div>
331+
<Separator
332+
activeLinklabel={activeLinklabel}
333+
className={clsx(
334+
styles['SubNav__heading-separator--subheading'],
335+
subHeadingIsActive && styles['SubNav__heading-separator--subheading-active'],
336+
)}
337+
/>
338+
<div
339+
className={clsx(
340+
styles['SubNav__heading-container'],
341+
styles['SubNav__subheading-container'],
342+
subHeadingIsActive && styles['SubNav__subheading-container-active'],
343+
)}
344+
>
345+
{SubHeadingChild}
346+
</div>
330347
</>
331348
)}
332349

333-
{isLarge || activeLinklabel ? <Separator activeLinklabel={activeLinklabel} /> : null}
350+
<Separator activeLinklabel={activeLinklabel} className={styles['SubNav__heading-separator--main']} />
334351

335352
{!isLarge && (!SubHeadingChild || subHeadingIsActive) && NarrowButton}
336353

‎packages/react/src/SubNav/SubNav.visual.spec.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,4 +1109,116 @@ test.describe('Visual Comparison: SubNav', () => {
11091109
await page.waitForTimeout(500)
11101110
await expect(page).toHaveScreenshot({fullPage: true})
11111111
})
1112+
1113+
test('SubNav / With no aria-current set', async ({page}) => {
1114+
await page.goto(
1115+
'http://localhost:6006/iframe.html?args=&id=components-subnav-features--no-active-links&viewMode=story',
1116+
)
1117+
1118+
await page.waitForTimeout(500)
1119+
await expect(page).toHaveScreenshot({fullPage: true})
1120+
})
1121+
1122+
test('SubNav / With no aria-current set (fr)', async ({page}) => {
1123+
await page.goto(
1124+
'http://localhost:6006/iframe.html?globals=locale%3Afr&args=&id=components-subnav-features--no-active-links&viewMode=story',
1125+
)
1126+
1127+
await page.waitForTimeout(500)
1128+
await expect(page).toHaveScreenshot({fullPage: true})
1129+
})
1130+
1131+
test('SubNav / With no aria-current set (de)', async ({page}) => {
1132+
await page.goto(
1133+
'http://localhost:6006/iframe.html?globals=locale%3Ade&args=&id=components-subnav-features--no-active-links&viewMode=story',
1134+
)
1135+
1136+
await page.waitForTimeout(500)
1137+
await expect(page).toHaveScreenshot({fullPage: true})
1138+
})
1139+
1140+
test('SubNav / With no aria-current set (ja)', async ({page}) => {
1141+
await page.goto(
1142+
'http://localhost:6006/iframe.html?globals=locale%3Aja&args=&id=components-subnav-features--no-active-links&viewMode=story',
1143+
)
1144+
1145+
await page.waitForTimeout(500)
1146+
await expect(page).toHaveScreenshot({fullPage: true})
1147+
})
1148+
1149+
test('SubNav / With no aria-current set (es)', async ({page}) => {
1150+
await page.goto(
1151+
'http://localhost:6006/iframe.html?globals=locale%3Aes&args=&id=components-subnav-features--no-active-links&viewMode=story',
1152+
)
1153+
1154+
await page.waitForTimeout(500)
1155+
await expect(page).toHaveScreenshot({fullPage: true})
1156+
})
1157+
1158+
test('SubNav / With no aria-current set (pt-BR)', async ({page}) => {
1159+
await page.goto(
1160+
'http://localhost:6006/iframe.html?globals=locale%3Apt-BR&args=&id=components-subnav-features--no-active-links&viewMode=story',
1161+
)
1162+
1163+
await page.waitForTimeout(500)
1164+
await expect(page).toHaveScreenshot({fullPage: true})
1165+
})
1166+
1167+
// eslint-disable-next-line i18n-text/no-en
1168+
test.describe('Mobile viewport test for With no aria-current set (narrow)', () => {
1169+
test.use({viewport: {width: 360, height: 800}})
1170+
test('SubNav / With no aria-current set (narrow)', async ({page}) => {
1171+
await page.goto(
1172+
'http://localhost:6006/iframe.html?args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1173+
)
1174+
1175+
await page.waitForTimeout(500)
1176+
await expect(page).toHaveScreenshot({fullPage: true})
1177+
})
1178+
1179+
test('SubNav / With no aria-current set (narrow) (fr)', async ({page}) => {
1180+
await page.goto(
1181+
'http://localhost:6006/iframe.html?globals=locale%3Afr&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1182+
)
1183+
1184+
await page.waitForTimeout(500)
1185+
await expect(page).toHaveScreenshot({fullPage: true})
1186+
})
1187+
1188+
test('SubNav / With no aria-current set (narrow) (de)', async ({page}) => {
1189+
await page.goto(
1190+
'http://localhost:6006/iframe.html?globals=locale%3Ade&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1191+
)
1192+
1193+
await page.waitForTimeout(500)
1194+
await expect(page).toHaveScreenshot({fullPage: true})
1195+
})
1196+
1197+
test('SubNav / With no aria-current set (narrow) (ja)', async ({page}) => {
1198+
await page.goto(
1199+
'http://localhost:6006/iframe.html?globals=locale%3Aja&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1200+
)
1201+
1202+
await page.waitForTimeout(500)
1203+
await expect(page).toHaveScreenshot({fullPage: true})
1204+
})
1205+
1206+
test('SubNav / With no aria-current set (narrow) (es)', async ({page}) => {
1207+
await page.goto(
1208+
'http://localhost:6006/iframe.html?globals=locale%3Aes&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1209+
)
1210+
1211+
await page.waitForTimeout(500)
1212+
await expect(page).toHaveScreenshot({fullPage: true})
1213+
})
1214+
1215+
test('SubNav / With no aria-current set (narrow) (pt-BR)', async ({page}) => {
1216+
await page.goto(
1217+
'http://localhost:6006/iframe.html?globals=locale%3Apt-BR&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
1218+
)
1219+
1220+
await page.waitForTimeout(500)
1221+
await expect(page).toHaveScreenshot({fullPage: true})
1222+
})
1223+
})
11121224
})

‎packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-3a534-bNav-With-no-aria-current-set-narrow-de-1-linux.png‎

31.3 KB
Loading

‎packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-477bc-bNav-With-no-aria-current-set-narrow-es-1-linux.png‎

31.3 KB
Loading

‎packages/react/src/SubNav/SubNav.visual.spec.ts-snapshots/Visual-Comparison-SubNav-Mobile-viewport-test-5c494-v-With-no-aria-current-set-narrow-pt-BR-1-linux.png‎

31.3 KB
Loading

0 commit comments

Comments
 (0)