diff --git a/.changeset/chilly-buckets-kick.md b/.changeset/chilly-buckets-kick.md new file mode 100644 index 00000000000..648c95d6945 --- /dev/null +++ b/.changeset/chilly-buckets-kick.md @@ -0,0 +1,6 @@ +--- +"@primer/react": minor +--- +ActionList.Group: deprecate `title` prop - please use `ActionList.GroupHeading` instead +ActionList.GroupHeading: update the warning to be an error if there is no explict `as` prop for list `role` action lists. +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 diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx index ea9cd611348..d5f52083392 100644 --- a/packages/react/src/ActionList/ActionList.features.stories.tsx +++ b/packages/react/src/ActionList/ActionList.features.stories.tsx @@ -47,7 +47,7 @@ export const WithVisualListHeading = () => ( Filter by - Path + Repositories {}}> @@ -75,7 +75,7 @@ export const WithVisualListHeading = () => ( - Advanced + Advanced {}}> diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index a6a8a431c7a..df745e7a939 100644 --- a/packages/react/src/ActionList/ActionList.test.tsx +++ b/packages/react/src/ActionList/ActionList.test.tsx @@ -249,6 +249,35 @@ describe('ActionList', () => { 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(() => + HTMLRender( + + + + + Trigger + + + + Group Heading + + + + + + + , + ), + ).toThrow( + "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.", + ) + expect(spy).toHaveBeenCalled() + spy.mockRestore() + }) + it('should render the ActionList.GroupHeading component as a heading with the given heading level', async () => { const container = HTMLRender( @@ -262,18 +291,22 @@ describe('ActionList', () => { expect(heading).toBeInTheDocument() expect(heading).toHaveTextContent('Group Heading') }) - it('should throw a warning if ActionList.Group is used without as prop when no role is specified (for list role)', async () => { - const spy = jest.spyOn(console, 'warn').mockImplementationOnce(() => {}) - - HTMLRender( - - Heading - - Group Heading - - , + it('should throw an error if ActionList.GroupHeading is used without an `as` prop when no role is specified (for list role)', async () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn()) + expect(() => + HTMLRender( + + Heading + + Group Heading + Item + + , + ), + ).toThrow( + "You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.", ) - expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalled() spy.mockRestore() }) it('should render the ActionList.GroupHeading component as a span (not a heading tag) when role is specified as listbox', async () => { diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index 6bc591b6810..f4c1e2e8105 100644 --- a/packages/react/src/ActionList/Group.tsx +++ b/packages/react/src/ActionList/Group.tsx @@ -9,7 +9,7 @@ import {default as Heading} from '../Heading' import type {ActionListHeadingProps} from './Heading' import {useSlots} from '../hooks/useSlots' import {defaultSxProp} from '../utils/defaultSxProp' -import {warning} from '../utils/warning' +import {invariant} from '../utils/invariant' export type ActionListGroupProps = { /** @@ -20,7 +20,7 @@ export type ActionListGroupProps = { */ variant?: 'subtle' | 'filled' /** - * Primary text which names a `Group`. + * @deprecated (Use `ActionList.GroupHeading` instead. i.e. → Group title) */ title?: string /** @@ -85,8 +85,10 @@ export const Group: React.FC> = ({ > {title && !slots.groupHeading ? ( - + // Escape hatch: supports old API in a non breaking way + ) : null} + {/* Supports new API ActionList.GroupHeading */} {!title && slots.groupHeading ? React.cloneElement(slots.groupHeading) : null} > = ({ ) } -export type GroupHeadingProps = Pick & +export type GroupHeadingProps = Pick & Omit & SxProp & React.HTMLAttributes & { as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' + _internalBackwardCompatibleTitle?: string } /** @@ -123,7 +126,8 @@ export type GroupHeadingProps = Pick> = ({ as, variant, - title, + // We are not recommending this prop to be used, it should only be used internally for incremental rollout. + _internalBackwardCompatibleTitle, auxiliaryText, children, sx = defaultSxProp, @@ -132,11 +136,21 @@ export const GroupHeading: React.FC> const {variant: listVariant, role: listRole} = React.useContext(ListContext) const {groupHeadingId} = React.useContext(GroupContext) // for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs - warning( - listRole === undefined && children !== undefined && as === undefined, + const missingAsForList = (listRole === undefined || listRole === 'list') && children !== undefined && as === undefined + const unnecessaryAsForListboxOrMenu = + listRole !== undefined && listRole !== 'list' && children !== undefined && as !== undefined + invariant( + // 'as' prop is required for list roles. ... + !missingAsForList, `You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.`, ) + invariant( + // 'as' prop on listbox or menu roles are not needed since they are rendered as divs and they could be misleading. + !unnecessaryAsForListboxOrMenu, + `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.`, + ) + const styles = { paddingY: '6px', paddingX: listVariant === 'full' ? 2 : 3, @@ -155,15 +169,17 @@ export const GroupHeading: React.FC> return ( <> - {listRole ? ( + {/* for listbox (SelectPanel) and menu (ActionMenu) roles, group titles are presentational. */} + {listRole && listRole !== 'list' ? ( ) : ( + // 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. <> (styles, sx)} {...props}> - {title ?? children} + {_internalBackwardCompatibleTitle ?? children} {auxiliaryText && {auxiliaryText}}