Fix separator rendering bug in SubNav

.changeset/poor-balloons-jam.md

@@ -0,0 +1,5 @@
+---
+'@primer/react-brand': patch
+---
+
+Fix layout shift in `SubNav` by ensuring separator visibility is determined pre-hydration.

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

@@ -508,3 +508,49 @@ AnchorNavVariantKeyboardNavigation.play = async ({canvasElement}) => {
   await userEvent.tab({shift: true})
   expect(getByRole('link', {name: 'Premium Support'})).toHaveFocus()
 }
+
+export const NoActiveLinks = args => (
+  <main>
+    <Box paddingBlockStart={64} backgroundColor="subtle" style={{position: 'relative', zIndex: 32}}></Box>
+    <SubNav {...args}>
+      <SubNav.Heading href="#">Features</SubNav.Heading>
+      <SubNav.Link href="#">Actions</SubNav.Link>
+      <SubNav.Link href="#">Packages</SubNav.Link>
+      <SubNav.Link href="#">Security</SubNav.Link>
+      <SubNav.Link href="#">Codespaces</SubNav.Link>
+      <SubNav.Link href="#">Copilot</SubNav.Link>
+      <SubNav.Link href="#">Code review</SubNav.Link>
+      <SubNav.Link href="#">Search</SubNav.Link>
+      <SubNav.Link href="#">Issues</SubNav.Link>
+      <SubNav.Link href="#">Discussions</SubNav.Link>
+      <SubNav.Action href="#">Get started</SubNav.Action>
+    </SubNav>
+    <Grid>
+      <Grid.Column>
+        <Hero align="center">
+          <Hero.Label>GitHub Features</Hero.Label>
+          <Hero.Heading>Choose the tools that work best for your team.</Hero.Heading>
+          <Hero.Description>
+            This story demonstrates the SubNav component when no links have aria-current=&quot;page&quot; set, which
+            should result in no separator being visible according to the test expectations.
+          </Hero.Description>
+          <Hero.PrimaryAction href="#">Explore features</Hero.PrimaryAction>
+        </Hero>
+      </Grid.Column>
+    </Grid>
+  </main>
+)
+NoActiveLinks.parameters = {
+  layout: 'fullscreen',
+}
+NoActiveLinks.storyName = 'With no aria-current set'
+
+export const NoActiveLinksNarrow = () => <NoActiveLinks />
+NoActiveLinksNarrow.parameters = {
+  layout: 'fullscreen',
+  viewport: {
+    defaultViewport: 'iphonex',
+  },
+}
+
+NoActiveLinksNarrow.storyName = 'With no aria-current set (narrow)'

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

@@ -82,8 +82,12 @@
 
 .SubNav__heading-separator {
   position: relative;
-  top: var(--base-size-2);
+  top: 0;
   color: var(--brand-color-text-muted);
+  min-width: var(--base-size-8);
+  min-height: var(--base-size-16);
+  display: inline-flex;
+  align-items: center;
 }
 
 /*
@@ -275,6 +279,12 @@
     animation: fade-in 0.3s var(--brand-animation-easing-glide) forwards;
   }
 
+  .SubNav__heading-separator--main:not(.SubNav__heading-separator--has-adjacent-label),
+  .SubNav__heading-separator--subheading-active,
+  .SubNav__subheading-container-active {
+    display: none;
+  }
+
   .SubNav__header-container {
     display: flex;
     width: 100%;
@@ -286,10 +296,6 @@
     z-index: 9998;
   }
 
-  .SubNav__heading-separator:not(.SubNav__heading-separator--has-adjacent-label) {
-    display: none;
-  }
-
   .SubNav__links-overlay {
     position: relative;
     display: flex;

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

@@ -19,6 +19,8 @@ declare const styles: {
   readonly "SubNav__heading-label": string;
   readonly "SubNav__heading-separator": string;
   readonly "SubNav__heading-separator--has-adjacent-label": string;
+  readonly "SubNav__heading-separator--main": string;
+  readonly "SubNav__heading-separator--subheading-active": string;
   readonly "SubNav__link": string;
   readonly "SubNav__link--expanded": string;
   readonly "SubNav__link--has-sub-menu": string;
@@ -38,6 +40,7 @@ declare const styles: {
   readonly "SubNav__sub-menu-list": string;
   readonly "SubNav__sub-menu-toggle": string;
   readonly "SubNav__subHeading": string;
+  readonly "SubNav__subheading-container-active": string;
   readonly "fade-in": string;
   readonly "fade-in-down": string;
 };

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

@@ -174,19 +174,6 @@ describe('SubNav', () => {
     expect(separator).toBeInTheDocument()
   })
 
-  it('does not render a separator when there are no links with `aria-current="page"` set', () => {
-    const {queryByRole} = render(
-      <MockSubNavFixture
-        data={[
-          {title: 'page one', href: '#page1'},
-          {title: 'page two', href: '#page2'},
-        ]}
-      />,
-    )
-
-    expect(queryByRole('separator', {hidden: true})).not.toBeInTheDocument()
-  })
-
   it('shows the aria-current text next to the button by default', () => {
     const {getByRole} = render(<MockSubNavFixture />)
 

packages/react/src/SubNav/SubNav.tsx

@@ -124,15 +124,17 @@ type SeparatorProps = {
   activeLinklabel?: string
 } & BaseProps<HTMLSpanElement>
 
-function Separator({activeLinklabel}: SeparatorProps) {
+function Separator({activeLinklabel, className, ...props}: SeparatorProps) {
   return (
     <span
       role="separator"
       className={clsx(
         styles['SubNav__heading-separator'],
         activeLinklabel && styles['SubNav__heading-separator--has-adjacent-label'],
+        className,
       )}
       aria-hidden
+      {...props}
     >
       <svg xmlns="http://www.w3.org/2000/svg" width="8" height="16" viewBox="0 0 8 16" fill="none" aria-hidden>
         <g clipPath="url(#clip0_50_1307)">
@@ -323,14 +325,29 @@ const _SubNavRoot = memo(
               <div ref={innerRootRef} className={styles['SubNav--header-container-outer']}>
                 <div className={styles['SubNav__header-container']}>
                   {HeadingChild && <div className={styles['SubNav__heading-container']}>{HeadingChild}</div>}
-                  {SubHeadingChild && (isLarge || !subHeadingIsActive) && (
+
+                  {SubHeadingChild && (
                     <>
-                      <Separator activeLinklabel={activeLinklabel} />
-                      <div className={styles['SubNav__heading-container']}>{SubHeadingChild}</div>
+                      <Separator
+                        activeLinklabel={activeLinklabel}
+                        className={clsx(
+                          styles['SubNav__heading-separator--subheading'],
+                          subHeadingIsActive && styles['SubNav__heading-separator--subheading-active'],
+                        )}
+                      />
+                      <div
+                        className={clsx(
+                          styles['SubNav__heading-container'],
+                          styles['SubNav__subheading-container'],
+                          subHeadingIsActive && styles['SubNav__subheading-container-active'],
+                        )}
+                      >
+                        {SubHeadingChild}
+                      </div>
                     </>
                   )}
 
-                  {isLarge || activeLinklabel ? <Separator activeLinklabel={activeLinklabel} /> : null}
+                  <Separator activeLinklabel={activeLinklabel} className={styles['SubNav__heading-separator--main']} />
 
                   {!isLarge && (!SubHeadingChild || subHeadingIsActive) && NarrowButton}
 

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

@@ -1109,4 +1109,116 @@ test.describe('Visual Comparison: SubNav', () => {
     await page.waitForTimeout(500)
     await expect(page).toHaveScreenshot({fullPage: true})
   })
+
+  test('SubNav / With no aria-current set', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('SubNav / With no aria-current set (fr)', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?globals=locale%3Afr&args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('SubNav / With no aria-current set (de)', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?globals=locale%3Ade&args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('SubNav / With no aria-current set (ja)', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?globals=locale%3Aja&args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('SubNav / With no aria-current set (es)', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?globals=locale%3Aes&args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  test('SubNav / With no aria-current set (pt-BR)', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?globals=locale%3Apt-BR&args=&id=components-subnav-features--no-active-links&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    await expect(page).toHaveScreenshot({fullPage: true})
+  })
+
+  // eslint-disable-next-line i18n-text/no-en
+  test.describe('Mobile viewport test for With no aria-current set (narrow)', () => {
+    test.use({viewport: {width: 360, height: 800}})
+    test('SubNav / With no aria-current set (narrow)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+
+    test('SubNav / With no aria-current set (narrow) (fr)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?globals=locale%3Afr&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+
+    test('SubNav / With no aria-current set (narrow) (de)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?globals=locale%3Ade&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+
+    test('SubNav / With no aria-current set (narrow) (ja)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?globals=locale%3Aja&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+
+    test('SubNav / With no aria-current set (narrow) (es)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?globals=locale%3Aes&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+
+    test('SubNav / With no aria-current set (narrow) (pt-BR)', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?globals=locale%3Apt-BR&args=&id=components-subnav-features--no-active-links-narrow&viewMode=story',
+      )
+
+      await page.waitForTimeout(500)
+      await expect(page).toHaveScreenshot({fullPage: true})
+    })
+  })
 })