Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/good-years-attack.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Remove the CSS modules feature flag from `Heading`
26 changes: 0 additions & 26 deletions e2e/components/Heading.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,6 @@ test.describe('Heading', () => {
for (const story of stories) {
test.describe(story.title, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: story.id,
globals: {
featureFlags: {
primer_react_css_modules_ga: true,
},
},
})

// Default state
expect(await page.screenshot()).toMatchSnapshot(`Heading.${story.title}.png`)
})

test('default (styled-components) @vrt', async ({page}) => {
await visit(page, {
id: story.id,
})
Expand All @@ -47,18 +33,6 @@ test.describe('Heading', () => {
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: story.id,
globals: {
featureFlags: {
primer_react_css_modules_ga: true,
},
},
})
await expect(page).toHaveNoViolations()
})

test('axe (styled-components) @aat', async ({page}) => {
await visit(page, {
id: story.id,
})
Expand Down
63 changes: 12 additions & 51 deletions packages/react/src/Heading/Heading.tsx
Original file line number Diff line number Diff line change
@@ -1,43 +1,18 @@
import {clsx} from 'clsx'
import React, {forwardRef, useEffect} from 'react'
import styled from 'styled-components'
import {get} from '../constants'
import {useRefObjectAsForwardedRef} from '../hooks'
import type {SxProp} from '../sx'
import sx from '../sx'
import type {ComponentProps} from '../utils/types'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
import classes from './Heading.module.css'
import {useFeatureFlag} from '../FeatureFlags'
import Box from '../Box'

type StyledHeadingProps = {
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
variant?: 'large' | 'medium' | 'small'
} & SxProp

const StyledHeading = styled.h2<StyledHeadingProps>`
font-weight: ${get('fontWeights.bold')};
font-size: ${get('fontSizes.5')};
margin: 0;

&:where([data-variant='large']) {
font: var(--text-title-shorthand-large, 600 32px / 1.5 ${get('fonts.normal')});
}

&:where([data-variant='medium']) {
font: var(--text-title-shorthand-medium, 600 20px / 1.6 ${get('fonts.normal')});
}

&:where([data-variant='small']) {
font: var(--text-title-shorthand-small, 600 16px / 1.5 ${get('fonts.normal')});
}

${sx};
`

const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}, forwardedRef) => {
const enabled = useFeatureFlag('primer_react_css_modules_ga')
const innerRef = React.useRef<HTMLHeadingElement>(null)
useRefObjectAsForwardedRef(forwardedRef, innerRef)

Expand All @@ -57,33 +32,19 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}
}, [innerRef])
}

if (enabled) {
if (props.sx) {
return (
<Box
as={Component}
className={clsx(className, classes.Heading)}
data-variant={variant}
{...props}
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
/>
)
}
return <Component className={clsx(className, classes.Heading)} data-variant={variant} {...props} ref={innerRef} />
if (props.sx) {
return (
<Box
as={Component}
className={clsx(className, classes.Heading)}
data-variant={variant}
{...props}
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
/>
)
}

return (
<StyledHeading
as={Component}
className={className}
data-variant={variant}
sx={sx}
{...props}
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
/>
)
return <Component className={clsx(className, classes.Heading)} data-variant={variant} {...props} ref={innerRef} />
}) as PolymorphicForwardRefComponent<'h2', StyledHeadingProps>

Heading.displayName = 'Heading'
Expand Down
75 changes: 25 additions & 50 deletions packages/react/src/Heading/__tests__/Heading.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {render, behavesAsComponent, checkExports} from '../../utils/testing'
import {render as HTMLRender, screen} from '@testing-library/react'
import axe from 'axe-core'
import ThemeProvider from '../../ThemeProvider'
import {FeatureFlags} from '../../FeatureFlags'

const theme = {
breakpoints: ['400px', '640px', '960px', '1280px'],
Expand Down Expand Up @@ -142,54 +141,30 @@ describe('Heading', () => {
).toHaveStyleRule('font-style', 'italic')
})

describe('with primer_react_css_modules_ga enabled', () => {
it('should only include css modules class', () => {
HTMLRender(
<FeatureFlags
flags={{
primer_react_css_modules_ga: true,
}}
>
<Heading>test</Heading>
</FeatureFlags>,
)
expect(screen.getByText('test')).toHaveClass('Heading')
// Note: this is the generated class name when styled-components is used
// for this component
expect(screen.getByText('test')).not.toHaveClass(/^Heading__StyledHeading/)
})

it('should support `className` on the outermost element', () => {
const {container} = HTMLRender(
<FeatureFlags
flags={{
primer_react_css_modules_ga: true,
}}
>
<Heading className="test">test</Heading>
</FeatureFlags>,
)
expect(container.firstChild).toHaveClass('test')
})

it('should support overrides with sx if provided', () => {
HTMLRender(
<FeatureFlags
flags={{
primer_react_css_modules_ga: true,
}}
>
<Heading
sx={{
fontWeight: '900',
}}
>
test
</Heading>
</FeatureFlags>,
)

expect(screen.getByText('test')).toHaveStyle('font-weight: 900')
})
it('should only include css modules class', () => {
HTMLRender(<Heading>test</Heading>)
expect(screen.getByText('test')).toHaveClass('Heading')
// Note: this is the generated class name when styled-components is used
// for this component
expect(screen.getByText('test')).not.toHaveClass(/^Heading__StyledHeading/)
})

it('should support `className` on the outermost element', () => {
const {container} = HTMLRender(<Heading className="test">test</Heading>)
expect(container.firstChild).toHaveClass('test')
})

it('should support overrides with sx if provided', () => {
HTMLRender(
<Heading
sx={{
fontWeight: '900',
}}
>
test
</Heading>,
)

expect(screen.getByText('test')).toHaveStyle('font-weight: 900')
})
})
39 changes: 12 additions & 27 deletions packages/react/src/NavList/__snapshots__/NavList.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,16 @@ exports[`NavList renders with groups 1`] = `
margin-top: 8px;
}

.c3 {
padding-top: 6px;
padding-bottom: 6px;
padding-left: 16px;
padding-right: 16px;
font-size: 12px;
font-weight: 600;
color: var(--fgColor-muted,var(--color-fg-muted,#656d76));
}

.c4 {
padding-inline-start: 0;
}
Expand Down Expand Up @@ -501,31 +511,6 @@ exports[`NavList renders with groups 1`] = `
word-break: break-word;
}

.c3 {
font-weight: 600;
font-size: 32px;
margin: 0;
padding-top: 6px;
padding-bottom: 6px;
padding-left: 16px;
padding-right: 16px;
font-size: 12px;
font-weight: 600;
color: var(--fgColor-muted,var(--color-fg-muted,#656d76));
}

.c3:where([data-variant='large']) {
font: var(--text-title-shorthand-large,600 32px / 1.5 -apple-system,BlinkMacSystemFont,"Segoe UI","Noto Sans",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji");
}

.c3:where([data-variant='medium']) {
font: var(--text-title-shorthand-medium,600 20px / 1.6 -apple-system,BlinkMacSystemFont,"Segoe UI","Noto Sans",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji");
}

.c3:where([data-variant='small']) {
font: var(--text-title-shorthand-small,600 16px / 1.5 -apple-system,BlinkMacSystemFont,"Segoe UI","Noto Sans",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji");
}

.c0 {
margin: 0;
padding-inline-start: 0;
Expand Down Expand Up @@ -869,7 +854,7 @@ exports[`NavList renders with groups 1`] = `
class="c2"
>
<h3
class="c3"
class="c3 Heading"
id=":r7:"
>
Overview
Expand Down Expand Up @@ -913,7 +898,7 @@ exports[`NavList renders with groups 1`] = `
class="c2"
>
<h3
class="c3"
class="c3 Heading"
id=":r9:"
>
Components
Expand Down