refactor(BranchName): update BranchName to use CSS Modules (#5040)

* refactor(BranchName): update BranchName to use CSS Modules * chore: add changeset * fix: use sx instead of defaultSxProp * chore: remove defaultSxProp * chore: experiment with forward ref * Remove feature flag duplication * Add test for `className` support in `BranchName` * refactor: update usage for forwardRef * chore: update fallthrough to use as prop * Update BranchName.displayname * test: add option to skip display name check --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Jon Rohan <yes@jonrohan.codes>
primer · Oct 31, 2024 · 8d9a357 · 8d9a357
1 parent 719def7
commit 8d9a357
Show file tree

Hide file tree

Showing 7 changed files with 189 additions and 105 deletions.
diff --git a/.changeset/four-schools-grin.md b/.changeset/four-schools-grin.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+Update BranchName to use CSS Modules behind feature flag
diff --git a/e2e/components/BranchName.test.ts b/e2e/components/BranchName.test.ts
@@ -2,110 +2,58 @@ import {test, expect} from '@playwright/test'
 import {visit} from '../test-helpers/storybook'
 import {themes} from '../test-helpers/themes'
 
-test.describe('BranchName', () => {
-  test.describe('Default', () => {
-    for (const theme of themes) {
-      test.describe(theme, () => {
-        test('default @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname--default',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-
-          // Default state
-          expect(await page.screenshot()).toMatchSnapshot(`BranchName.Default.${theme}.png`)
-
-          // Focus state
-          await page.keyboard.press('Tab')
-          expect(await page.screenshot()).toMatchSnapshot(`BranchName.Default.${theme}.focus.png`)
-        })
+const stories = [
+  {
+    title: 'Default',
+    id: 'components-branchname--default',
+    focus: true,
+  },
+  {
+    title: 'Not A Link',
+    id: 'components-branchname-features--not-a-link',
+    focus: false,
+  },
+  {
+    title: 'With A Branch Icon',
+    id: 'components-branchname-features--with-branch-icon',
+    focus: false,
+  },
+] as const
 
-        test('axe @aat', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname--default',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await expect(page).toHaveNoViolations({
-            rules: {
-              'color-contrast': {
-                enabled: theme !== 'dark_dimmed',
+test.describe('BranchName', () => {
+  for (const story of stories) {
+    test.describe(story.title, () => {
+      for (const theme of themes) {
+        test.describe(theme, () => {
+          test('default @vrt', async ({page}) => {
+            await visit(page, {
+              id: story.id,
+              globals: {
+                colorScheme: theme,
               },
-            },
-          })
-        })
-      })
-    }
-  })
-
-  test.describe('Not A Link', () => {
-    for (const theme of themes) {
-      test.describe(theme, () => {
-        test('default @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname-features--not-a-link',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-
-          // Default state
-          expect(await page.screenshot()).toMatchSnapshot(`BranchName.Not A Link.${theme}.png`)
-        })
+            })
 
-        test('axe @aat', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname-features--not-a-link',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await expect(page).toHaveNoViolations({
-            rules: {
-              'color-contrast': {
-                enabled: theme !== 'dark_dimmed',
-              },
-            },
-          })
-        })
-      })
-    }
-  })
+            // Default state
+            expect(await page.screenshot()).toMatchSnapshot(`BranchName.${story.title}.${theme}.png`)
 
-  test.describe('With A Branch Icon', () => {
-    for (const theme of themes) {
-      test.describe(theme, () => {
-        test('default @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname-features--with-branch-icon',
-            globals: {
-              colorScheme: theme,
-            },
+            // Focus state
+            if (story.focus) {
+              await page.keyboard.press('Tab')
+              expect(await page.screenshot()).toMatchSnapshot(`BranchName.${story.title}.${theme}.focus.png`)
+            }
           })
 
-          // Default state
-          expect(await page.screenshot()).toMatchSnapshot(`BranchName.With A Branch Icon.${theme}.png`)
-        })
-
-        test('axe @aat', async ({page}) => {
-          await visit(page, {
-            id: 'components-branchname-features--with-branch-icon',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await expect(page).toHaveNoViolations({
-            rules: {
-              'color-contrast': {
-                enabled: theme !== 'dark_dimmed',
+          test('axe @aat', async ({page}) => {
+            await visit(page, {
+              id: story.id,
+              globals: {
+                colorScheme: theme,
               },
-            },
+            })
+            await expect(page).toHaveNoViolations()
           })
         })
-      })
-    }
-  })
+      }
+    })
+  }
 })
diff --git a/packages/react/src/BranchName/BranchName.module.css b/packages/react/src/BranchName/BranchName.module.css
@@ -0,0 +1,14 @@
+.BranchName {
+  display: inline-block;
+  padding: var(--base-size-2) var(--base-size-6);
+  font-family: var(--fontStack-monospace);
+  font-size: var(--text-body-size-small);
+  color: var(--fgColor-link);
+  text-decoration: none;
+  background-color: var(--bgColor-accent-muted);
+  border-radius: var(--borderRadius-medium);
+
+  &:is(:not(a)) {
+    color: var(--fgColor-muted);
+  }
+}
diff --git a/packages/react/src/BranchName/BranchName.tsx b/packages/react/src/BranchName/BranchName.tsx
@@ -1,10 +1,14 @@
+import React, {type ForwardedRef} from 'react'
+import {clsx} from 'clsx'
 import styled from 'styled-components'
 import {get} from '../constants'
 import type {SxProp} from '../sx'
 import sx from '../sx'
-import type {ComponentProps} from '../utils/types'
+import {useFeatureFlag} from '../FeatureFlags'
+import Box from '../Box'
+import classes from './BranchName.module.css'
 
-const BranchName = styled.a<SxProp>`
+const StyledBranchName = styled.a<SxProp>`
   display: inline-block;
   padding: 2px 6px;
   font-size: var(--text-body-size-small, ${get('fontSizes.0')});
@@ -19,5 +23,50 @@ const BranchName = styled.a<SxProp>`
   ${sx};
 `
 
-export type BranchNameProps = ComponentProps<typeof BranchName>
-export default BranchName
+type BranchNameProps<As extends React.ElementType> = {
+  as?: As
+} & DistributiveOmit<React.ComponentPropsWithRef<React.ElementType extends As ? 'a' : As>, 'as'> &
+  SxProp
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function BranchName<As extends React.ElementType>(props: BranchNameProps<As>, ref: ForwardedRef<any>) {
+  const {as: BaseComponent = 'a', className, children, sx, ...rest} = props
+  const enabled = useFeatureFlag('primer_react_css_modules_team')
+
+  if (enabled) {
+    if (sx) {
+      return (
+        <Box {...rest} ref={ref} as={BaseComponent} className={clsx(className, classes.BranchName)} sx={sx}>
+          {children}
+        </Box>
+      )
+    }
+
+    return (
+      <BaseComponent {...rest} ref={ref} className={clsx(className, classes.BranchName)}>
+        {children}
+      </BaseComponent>
+    )
+  }
+
+  return (
+    <StyledBranchName {...rest} as={BaseComponent} ref={ref} className={className} sx={sx}>
+      {children}
+    </StyledBranchName>
+  )
+}
+
+// eslint-disable-next-line @typescript-eslint/ban-types
+type FixedForwardRef = <T, P = {}>(
+  render: (props: P, ref: React.Ref<T>) => React.ReactNode,
+) => (props: P & React.RefAttributes<T>) => React.ReactNode
+
+const fixedForwardRef = React.forwardRef as FixedForwardRef
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type DistributiveOmit<T, TOmitted extends PropertyKey> = T extends any ? Omit<T, TOmitted> : never
+
+BranchName.displayName = 'BranchName'
+
+export type {BranchNameProps}
+export default fixedForwardRef(BranchName)
diff --git a/packages/react/src/BranchName/__tests__/BranchName.test.tsx b/packages/react/src/BranchName/__tests__/BranchName.test.tsx
@@ -3,9 +3,15 @@ import BranchName from '../BranchName'
 import {render, behavesAsComponent, checkExports} from '../../utils/testing'
 import {render as HTMLRender} from '@testing-library/react'
 import axe from 'axe-core'
+import {FeatureFlags} from '../../FeatureFlags'
 
 describe('BranchName', () => {
-  behavesAsComponent({Component: BranchName})
+  behavesAsComponent({
+    Component: BranchName,
+    options: {
+      skipDisplayName: true,
+    },
+  })
 
   checkExports('BranchName', {
     default: BranchName,
@@ -20,4 +26,23 @@ describe('BranchName', () => {
   it('renders an <a> by default', () => {
     expect(render(<BranchName />).type).toEqual('a')
   })
+
+  it('should support `className` on the outermost element', () => {
+    const Element = () => <BranchName className={'test-class-name'} />
+    const FeatureFlagElement = () => {
+      return (
+        <FeatureFlags
+          flags={{
+            primer_react_css_modules_team: true,
+            primer_react_css_modules_staff: true,
+            primer_react_css_modules_ga: true,
+          }}
+        >
+          <Element />
+        </FeatureFlags>
+      )
+    }
+    expect(HTMLRender(<Element />).container.firstChild).toHaveClass('test-class-name')
+    expect(HTMLRender(<FeatureFlagElement />).container.firstChild).toHaveClass('test-class-name')
+  })
 })
diff --git a/packages/react/src/BranchName/__tests__/BranchName.types.test.tsx b/packages/react/src/BranchName/__tests__/BranchName.types.test.tsx
@@ -9,3 +9,43 @@ export function shouldNotAcceptSystemProps() {
   // @ts-expect-error system props should not be accepted
   return <BranchName backgroundColor="thistle" />
 }
+
+export function shouldAcceptAs() {
+  return (
+    <BranchName
+      as="button"
+      onClick={event => {
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        type test = Expect<Equal<typeof event, React.MouseEvent<HTMLButtonElement, MouseEvent>>>
+      }}
+    />
+  )
+}
+
+export function defaultAsIsAnchor() {
+  return (
+    <BranchName
+      onClick={event => {
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        type test = Expect<Equal<typeof event, React.MouseEvent<HTMLAnchorElement, MouseEvent>>>
+      }}
+    />
+  )
+}
+
+export function ShouldAcceptRef() {
+  const ref = React.useRef<HTMLButtonElement>(null)
+  return (
+    <BranchName
+      as="button"
+      ref={ref}
+      onClick={event => {
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        type test = Expect<Equal<typeof event, React.MouseEvent<HTMLButtonElement, MouseEvent>>>
+      }}
+    />
+  )
+}
+
+type Expect<T extends true> = T
+type Equal<X, Y> = (<T>() => T extends X ? 1 : 2) extends <T>() => T extends Y ? 1 : 2 ? true : false
diff --git a/packages/react/src/utils/testing.tsx b/packages/react/src/utils/testing.tsx
@@ -193,6 +193,7 @@ export function unloadCSS(path: string) {
 interface Options {
   skipAs?: boolean
   skipSx?: boolean
+  skipDisplayName?: boolean
 }
 
 interface BehavesAsComponent {
@@ -221,9 +222,11 @@ export function behavesAsComponent({Component, toRender, options}: BehavesAsComp
     })
   }
 
-  it('sets a valid displayName', () => {
-    expect(Component.displayName).toMatch(COMPONENT_DISPLAY_NAME_REGEX)
-  })
+  if (!options.skipDisplayName) {
+    it('sets a valid displayName', () => {
+      expect(Component.displayName).toMatch(COMPONENT_DISPLAY_NAME_REGEX)
+    })
+  }
 }
 
 // eslint-disable-next-line @typescript-eslint/no-explicit-any