Skip to content

Commit

Permalink
refactor(BranchName): update BranchName to use CSS Modules (#5040)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
3 people authored Oct 31, 2024
1 parent 719def7 commit 8d9a357
Show file tree
Hide file tree
Showing 7 changed files with 189 additions and 105 deletions.
5 changes: 5 additions & 0 deletions .changeset/four-schools-grin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Update BranchName to use CSS Modules behind feature flag
142 changes: 45 additions & 97 deletions e2e/components/BranchName.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
})
}
})
}
})
}
})
14 changes: 14 additions & 0 deletions packages/react/src/BranchName/BranchName.module.css
Original file line number Diff line number Diff line change
@@ -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);
}
}
57 changes: 53 additions & 4 deletions packages/react/src/BranchName/BranchName.tsx
Original file line number Diff line number Diff line change
@@ -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')});
Expand All @@ -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)
27 changes: 26 additions & 1 deletion packages/react/src/BranchName/__tests__/BranchName.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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')
})
})
40 changes: 40 additions & 0 deletions packages/react/src/BranchName/__tests__/BranchName.types.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 6 additions & 3 deletions packages/react/src/utils/testing.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ export function unloadCSS(path: string) {
interface Options {
skipAs?: boolean
skipSx?: boolean
skipDisplayName?: boolean
}

interface BehavesAsComponent {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8d9a357

Please sign in to comment.