Skip to content

Commit c2557d1

Browse files
broccolinisoupbroccolinisoup
authored
ActionList.GroupHeading roll out improvements (#4395)
* ActionList.GroupHeading roll out improvements * fix tests * add a deprecated TS notice * Create chilly-buckets-kick.md * messed up the conflict * code review comment address * fix test and add another case * add an underscore to the internal bc title
1 parent 4b1bac1 commit c2557d1

File tree

4 files changed

+78
-23
lines changed

4 files changed

+78
-23
lines changed

‎.changeset/chilly-buckets-kick.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@primer/react": minor
3+
---
4+
ActionList.Group: deprecate `title` prop - please use `ActionList.GroupHeading` instead
5+
ActionList.GroupHeading: update the warning to be an error if there is no explict `as` prop for list `role` action lists.
6+
ActionList.GroupHeading: There shouldn't be an `as` prop on `ActionList.GroupHeading` for `listbox` or `menu` role action lists. console.error if there is one

‎packages/react/src/ActionList/ActionList.features.stories.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export const WithVisualListHeading = () => (
4747
<ActionList>
4848
<ActionList.Heading as="h2">Filter by</ActionList.Heading>
4949
<ActionList.Group>
50-
<ActionList.GroupHeading>Path</ActionList.GroupHeading>
50+
<ActionList.GroupHeading as="h4">Repositories</ActionList.GroupHeading>
5151
<ActionList.Item onClick={() => {}}>
5252
<ActionList.LeadingVisual>
5353
<FileDirectoryIcon />
@@ -75,7 +75,7 @@ export const WithVisualListHeading = () => (
7575
</ActionList.Group>
7676

7777
<ActionList.Group>
78-
<ActionList.GroupHeading>Advanced</ActionList.GroupHeading>
78+
<ActionList.GroupHeading as="h4">Advanced</ActionList.GroupHeading>
7979
<ActionList.Item onClick={() => {}}>
8080
<ActionList.LeadingVisual>
8181
<PlusCircleIcon />

‎packages/react/src/ActionList/ActionList.test.tsx‎

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,35 @@ describe('ActionList', () => {
249249
expect(spy).toHaveBeenCalled()
250250
spy.mockRestore()
251251
})
252+
253+
it('should throw an error when ActionList.GroupHeading has an `as` prop when it is used within ActionMenu context', async () => {
254+
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
255+
expect(() =>
256+
HTMLRender(
257+
<ThemeProvider theme={theme}>
258+
<SSRProvider>
259+
<BaseStyles>
260+
<ActionMenu open={true}>
261+
<ActionMenu.Button>Trigger</ActionMenu.Button>
262+
<ActionMenu.Overlay>
263+
<ActionList>
264+
<ActionList.Group>
265+
<ActionList.GroupHeading as="h2">Group Heading</ActionList.GroupHeading>
266+
</ActionList.Group>
267+
</ActionList>
268+
</ActionMenu.Overlay>
269+
</ActionMenu>
270+
</BaseStyles>
271+
</SSRProvider>
272+
</ThemeProvider>,
273+
),
274+
).toThrow(
275+
"Looks like you are trying to set a heading level to a menu role. Group headings for menu type action lists are for representational purposes, and rendered as divs. Therefore they don't need a heading level.",
276+
)
277+
expect(spy).toHaveBeenCalled()
278+
spy.mockRestore()
279+
})
280+
252281
it('should render the ActionList.GroupHeading component as a heading with the given heading level', async () => {
253282
const container = HTMLRender(
254283
<ActionList>
@@ -262,18 +291,22 @@ describe('ActionList', () => {
262291
expect(heading).toBeInTheDocument()
263292
expect(heading).toHaveTextContent('Group Heading')
264293
})
265-
it('should throw a warning if ActionList.Group is used without as prop when no role is specified (for list role)', async () => {
266-
const spy = jest.spyOn(console, 'warn').mockImplementationOnce(() => {})
267-
268-
HTMLRender(
269-
<ActionList>
270-
<ActionList.Heading as="h1">Heading</ActionList.Heading>
271-
<ActionList.Group>
272-
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
273-
</ActionList.Group>
274-
</ActionList>,
294+
it('should throw an error if ActionList.GroupHeading is used without an `as` prop when no role is specified (for list role)', async () => {
295+
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
296+
expect(() =>
297+
HTMLRender(
298+
<ActionList>
299+
<ActionList.Heading as="h1">Heading</ActionList.Heading>
300+
<ActionList.Group>
301+
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
302+
<ActionList.Item>Item</ActionList.Item>
303+
</ActionList.Group>
304+
</ActionList>,
305+
),
306+
).toThrow(
307+
"You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.",
275308
)
276-
expect(spy).toHaveBeenCalledTimes(1)
309+
expect(spy).toHaveBeenCalled()
277310
spy.mockRestore()
278311
})
279312
it('should render the ActionList.GroupHeading component as a span (not a heading tag) when role is specified as listbox', async () => {

‎packages/react/src/ActionList/Group.tsx‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {default as Heading} from '../Heading'
99
import type {ActionListHeadingProps} from './Heading'
1010
import {useSlots} from '../hooks/useSlots'
1111
import {defaultSxProp} from '../utils/defaultSxProp'
12-
import {warning} from '../utils/warning'
12+
import {invariant} from '../utils/invariant'
1313

1414
export type ActionListGroupProps = {
1515
/**
@@ -20,7 +20,7 @@ export type ActionListGroupProps = {
2020
*/
2121
variant?: 'subtle' | 'filled'
2222
/**
23-
* Primary text which names a `Group`.
23+
* @deprecated (Use `ActionList.GroupHeading` instead. i.e. <ActionList.Group title="Group title"> → <ActionList.GroupHeading>Group title</ActionList.GroupHeading>)
2424
*/
2525
title?: string
2626
/**
@@ -85,8 +85,10 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
8585
>
8686
<GroupContext.Provider value={{selectionVariant, groupHeadingId}}>
8787
{title && !slots.groupHeading ? (
88-
<GroupHeading title={title} variant={variant} auxiliaryText={auxiliaryText} />
88+
// Escape hatch: supports old API <ActionList.Group title="group title"> in a non breaking way
89+
<GroupHeading variant={variant} auxiliaryText={auxiliaryText} _internalBackwardCompatibleTitle={title} />
8990
) : null}
91+
{/* Supports new API ActionList.GroupHeading */}
9092
{!title && slots.groupHeading ? React.cloneElement(slots.groupHeading) : null}
9193
<Box
9294
as="ul"
@@ -105,11 +107,12 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
105107
)
106108
}
107109

108-
export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' | 'auxiliaryText'> &
110+
export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliaryText'> &
109111
Omit<ActionListHeadingProps, 'as'> &
110112
SxProp &
111113
React.HTMLAttributes<HTMLElement> & {
112114
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
115+
_internalBackwardCompatibleTitle?: string
113116
}
114117

115118
/**
@@ -123,7 +126,8 @@ export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' |
123126
export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>> = ({
124127
as,
125128
variant,
126-
title,
129+
// We are not recommending this prop to be used, it should only be used internally for incremental rollout.
130+
_internalBackwardCompatibleTitle,
127131
auxiliaryText,
128132
children,
129133
sx = defaultSxProp,
@@ -132,11 +136,21 @@ export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>>
132136
const {variant: listVariant, role: listRole} = React.useContext(ListContext)
133137
const {groupHeadingId} = React.useContext(GroupContext)
134138
// for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs
135-
warning(
136-
listRole === undefined && children !== undefined && as === undefined,
139+
const missingAsForList = (listRole === undefined || listRole === 'list') && children !== undefined && as === undefined
140+
const unnecessaryAsForListboxOrMenu =
141+
listRole !== undefined && listRole !== 'list' && children !== undefined && as !== undefined
142+
invariant(
143+
// 'as' prop is required for list roles. <GroupHeading as="h2">...</GroupHeading>
144+
!missingAsForList,
137145
`You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.`,
138146
)
139147

148+
invariant(
149+
// 'as' prop on listbox or menu roles are not needed since they are rendered as divs and they could be misleading.
150+
!unnecessaryAsForListboxOrMenu,
151+
`Looks like you are trying to set a heading level to a ${listRole} role. Group headings for ${listRole} type action lists are for representational purposes, and rendered as divs. Therefore they don't need a heading level.`,
152+
)
153+
140154
const styles = {
141155
paddingY: '6px',
142156
paddingX: listVariant === 'full' ? 2 : 3,
@@ -155,15 +169,17 @@ export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>>
155169

156170
return (
157171
<>
158-
{listRole ? (
172+
{/* for listbox (SelectPanel) and menu (ActionMenu) roles, group titles are presentational. */}
173+
{listRole && listRole !== 'list' ? (
159174
<Box sx={styles} role="presentation" aria-hidden="true" {...props}>
160-
<span id={groupHeadingId}>{title ?? children}</span>
175+
<span id={groupHeadingId}>{_internalBackwardCompatibleTitle ?? children}</span>
161176
{auxiliaryText && <span>{auxiliaryText}</span>}
162177
</Box>
163178
) : (
179+
// for explicit (role="list" is passed as prop) and implicit list roles (ActionList ins rendered as list by default), group titles are proper heading tags.
164180
<>
165181
<Heading as={as || 'h3'} id={groupHeadingId} sx={merge<BetterSystemStyleObject>(styles, sx)} {...props}>
166-
{title ?? children}
182+
{_internalBackwardCompatibleTitle ?? children}
167183
</Heading>
168184
{auxiliaryText && <span>{auxiliaryText}</span>}
169185
</>

0 commit comments

Comments
 (0)