Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ActionList.Heading): Convert to CSS Modules #5367

Merged
merged 9 commits into from
Dec 6, 2024
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/new-pans-shout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Convert ActionList.Heading to CSS Modules
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
28 changes: 28 additions & 0 deletions e2e/components/ActionList.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,4 +712,32 @@ test.describe('ActionList', () => {
})
}
})

test.describe('Heading with Classname', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-actionlist-dev--heading-custom-classname',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot()).toMatchSnapshot(`Heading with Classname.${theme}.png`)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-actionlist-dev--heading-custom-classname',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations()
})
})
}
})
})
13 changes: 13 additions & 0 deletions packages/react/src/ActionList/ActionList.dev.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,16 @@ export const GroupHeadingCustomClassname = () => (
</ActionList.Group>
</ActionList>
)

export const HeadingCustomClassname = () => (
<ActionList>
<ActionList.Heading className="testCustomClassnameColor" as="h2">
Filter by
</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading as="h3">Repositories</ActionList.GroupHeading>
<ActionList.Item onClick={() => {}}>app/assets/modules</ActionList.Item>
<ActionList.Item onClick={() => {}}>src/react/components</ActionList.Item>
</ActionList.Group>
</ActionList>
)
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export const SimpleList = () => (

export const WithVisualListHeading = () => (
<ActionList>
<ActionList.Heading as="h2">Filter by</ActionList.Heading>
<ActionList.Heading as="h2" size="small">
Filter by
</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading as="h3">Repositories</ActionList.GroupHeading>
<ActionList.Item onClick={() => {}}>
Expand Down
46 changes: 0 additions & 46 deletions packages/react/src/ActionList/ActionList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -237,52 +237,6 @@ describe('ActionList', () => {
expect(onClick).toHaveBeenCalled()
})

it('should render the ActionList.Heading component as a heading with the given heading level', async () => {
const container = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
</ActionList>,
)
const heading = container.getByRole('heading', {level: 1})
expect(heading).toBeInTheDocument()
expect(heading).toHaveTextContent('Heading')
})
it('should label the action list with the heading id', async () => {
const {container, getByRole} = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)
const list = container.querySelector('ul')
const heading = getByRole('heading', {level: 1})
expect(list).toHaveAttribute('aria-labelledby', heading.id)
})
it('should throw an error when ActionList.Heading is used within ActionMenu context', async () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
expect(() =>
HTMLRender(
<ThemeProvider theme={theme}>
<BaseStyles>
<ActionMenu open={true}>
<ActionMenu.Button>Trigger</ActionMenu.Button>
<ActionMenu.Overlay>
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Item>Item</ActionList.Item>
</ActionList>
</ActionMenu.Overlay>
</ActionMenu>
</BaseStyles>
</ThemeProvider>,
),
).toThrow(
"ActionList.Heading shouldn't be used within an ActionMenu container. Menus are labelled by the menu button's name.",
)
expect(spy).toHaveBeenCalled()
spy.mockRestore()
})

it('should throw an error when ActionList.GroupHeading has an `as` prop when it is used within ActionMenu context', async () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
expect(() =>
Expand Down
12 changes: 12 additions & 0 deletions packages/react/src/ActionList/Heading.module.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
.ActionListHeader {
margin-block-end: var(--base-size-8);

&:where([data-list-variant='full']) {
margin-inline-start: var(--base-size-8);
}

&:where([data-list-variant='inset']) {
/* stylelint-disable-next-line primer/spacing */
margin-inline-start: calc(var(--control-medium-paddingInline-condensed) + var(--base-size-8));
}
}
83 changes: 83 additions & 0 deletions packages/react/src/ActionList/Heading.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import {render as HTMLRender} from '@testing-library/react'
import React from 'react'
import theme from '../theme'
import {ActionList} from '.'
import {BaseStyles, ThemeProvider, ActionMenu} from '..'
import {FeatureFlags} from '../FeatureFlags'

describe('ActionList.Heading', () => {
it('should render the ActionList.Heading component as a heading with the given heading level', async () => {
const container = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
</ActionList>,
)
const heading = container.getByRole('heading', {level: 1})
expect(heading).toBeInTheDocument()
expect(heading).toHaveTextContent('Heading')
})

it('should label the action list with the heading id', async () => {
const {container, getByRole} = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)
const list = container.querySelector('ul')
const heading = getByRole('heading', {level: 1})
expect(list).toHaveAttribute('aria-labelledby', heading.id)
})

it('should throw an error when ActionList.Heading is used within ActionMenu context', async () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
expect(() =>
HTMLRender(
<ThemeProvider theme={theme}>
<BaseStyles>
<ActionMenu open={true}>
<ActionMenu.Button>Trigger</ActionMenu.Button>
<ActionMenu.Overlay>
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Item>Item</ActionList.Item>
</ActionList>
</ActionMenu.Overlay>
</ActionMenu>
</BaseStyles>
</ThemeProvider>,
),
).toThrow(
"ActionList.Heading shouldn't be used within an ActionMenu container. Menus are labelled by the menu button's name.",
)
expect(spy).toHaveBeenCalled()
spy.mockRestore()
})

it('should support a custom `className` on the outermost element', () => {
const Element = () => {
return (
<ActionList>
<ActionList.Heading as="h2" className="test-class-name">
Filter by
</ActionList.Heading>
</ActionList>
)
}
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(<FeatureFlagElement />).container.querySelector('h2')).toHaveClass('test-class-name')
expect(HTMLRender(<Element />).container.querySelector('h2')).toHaveClass('test-class-name')
})
})
63 changes: 52 additions & 11 deletions packages/react/src/ActionList/Heading.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,26 @@ import {ListContext} from './shared'
import VisuallyHidden from '../_VisuallyHidden'
import {ActionListContainerContext} from './ActionListContainerContext'
import {invariant} from '../utils/invariant'
import {clsx} from 'clsx'
import {useFeatureFlag} from '../FeatureFlags'
import classes from './Heading.module.css'

type HeadingLevels = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
type HeadingVariants = 'large' | 'medium' | 'small'
export type ActionListHeadingProps = {
as: HeadingLevels
size?: HeadingVariants
visuallyHidden?: boolean
className?: string
} & SxProp

export const Heading = forwardRef(
({as, children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => {
({as, size, children, sx = defaultSxProp, visuallyHidden = false, className, ...props}, forwardedRef) => {
const innerRef = React.useRef<HTMLHeadingElement>(null)
useRefObjectAsForwardedRef(forwardedRef, innerRef)

const enabled = useFeatureFlag('primer_react_css_modules_team')

const {headingId: headingId, variant: listVariant} = React.useContext(ListContext)
const {container} = React.useContext(ActionListContainerContext)

Expand All @@ -37,16 +45,49 @@ export const Heading = forwardRef(

return (
<VisuallyHidden isVisible={!visuallyHidden}>
<HeadingComponent
as={as}
ref={innerRef}
// use custom id if it is provided. Otherwise, use the id from the context
id={props.id ?? headingId}
sx={merge<BetterSystemStyleObject>(styles, sx)}
{...props}
>
{children}
</HeadingComponent>
{enabled ? (
sx !== defaultSxProp ? (
<HeadingComponent
as={as}
variant={size}
ref={innerRef}
// use custom id if it is provided. Otherwise, use the id from the context
id={props.id ?? headingId}
className={clsx(className, classes.ActionListHeader)}
data-list-variant={listVariant}
sx={sx}
{...props}
>
{children}
</HeadingComponent>
) : (
<HeadingComponent
as={as}
variant={size}
ref={innerRef}
// use custom id if it is provided. Otherwise, use the id from the context
id={props.id ?? headingId}
className={clsx(className, classes.ActionListHeader)}
data-list-variant={listVariant}
{...props}
>
{children}
</HeadingComponent>
)
) : (
<HeadingComponent
as={as}
variant={size}
ref={innerRef}
// use custom id if it is provided. Otherwise, use the id from the context
id={props.id ?? headingId}
sx={merge<BetterSystemStyleObject>(styles, sx)}
className={className}
{...props}
>
{children}
</HeadingComponent>
)}
</VisuallyHidden>
)
},
Expand Down
Loading