From cbcd7b590a9b8c1bf2cd48ad01053459252cfb63 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 15 Mar 2024 10:20:49 +1000 Subject: [PATCH 1/8] ActionList.GroupHeading roll out improvements --- .../ActionList.features.stories.tsx | 6 ++-- packages/react/src/ActionList/Group.tsx | 34 ++++++++++++++----- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx index 3e5b9a3266e..38c9d824ec4 100644 --- a/packages/react/src/ActionList/ActionList.features.stories.tsx +++ b/packages/react/src/ActionList/ActionList.features.stories.tsx @@ -46,7 +46,8 @@ export const SimpleList = () => ( export const WithVisualListHeading = () => ( Filter by - + + Repositories {}}> @@ -73,7 +74,8 @@ export const WithVisualListHeading = () => ( - + + Advanced {}}> diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index 6bc591b6810..0f26de4e9e2 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 = { /** @@ -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' + unsafeTitle?: 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. + unsafeTitle, 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} + {unsafeTitle ?? children} {auxiliaryText && {auxiliaryText}} From 86823a33ddbe506502768db8ac9dfea679b9bfb6 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 15 Mar 2024 12:00:41 +1000 Subject: [PATCH 2/8] fix tests --- packages/react/src/ActionList/ActionList.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index a6a8a431c7a..e2a5df96c12 100644 --- a/packages/react/src/ActionList/ActionList.test.tsx +++ b/packages/react/src/ActionList/ActionList.test.tsx @@ -262,8 +262,8 @@ 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(() => {}) + 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').mockImplementationOnce(() => {}) HTMLRender( @@ -273,7 +273,7 @@ describe('ActionList', () => { , ) - 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 () => { From f94e8d28b7a0869c3649fc973b432b1f5f9f699b Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 18 Mar 2024 11:51:08 +1000 Subject: [PATCH 3/8] add a deprecated TS notice --- packages/react/src/ActionList/Group.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index 0f26de4e9e2..98892d2c256 100644 --- a/packages/react/src/ActionList/Group.tsx +++ b/packages/react/src/ActionList/Group.tsx @@ -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 /** From 9b21aa0ecca85ba55c97449dc452907c506a4724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Thu, 21 Mar 2024 11:33:17 +1000 Subject: [PATCH 4/8] Create chilly-buckets-kick.md --- .changeset/chilly-buckets-kick.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/chilly-buckets-kick.md 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 From 8eabc6ea2a68a4c7f2a386d566c19136ff5c8c66 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 27 Mar 2024 10:19:47 +1000 Subject: [PATCH 5/8] messed up the conflict --- packages/react/src/ActionList/ActionList.features.stories.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx index 1373e6bd267..d5f52083392 100644 --- a/packages/react/src/ActionList/ActionList.features.stories.tsx +++ b/packages/react/src/ActionList/ActionList.features.stories.tsx @@ -75,11 +75,7 @@ export const WithVisualListHeading = () => ( -<<<<<<< HEAD Advanced -======= - Advanced ->>>>>>> main {}}> From 4f154cfb312a2be99c18e59db38751da8c9ca6fa Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 27 Mar 2024 10:48:59 +1000 Subject: [PATCH 6/8] code review comment address --- .../react/src/ActionList/ActionList.test.tsx | 22 +++++++++++-------- packages/react/src/ActionList/Group.tsx | 10 ++++----- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index e2a5df96c12..33e42c2082f 100644 --- a/packages/react/src/ActionList/ActionList.test.tsx +++ b/packages/react/src/ActionList/ActionList.test.tsx @@ -263,15 +263,19 @@ describe('ActionList', () => { expect(heading).toHaveTextContent('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').mockImplementationOnce(() => {}) - - HTMLRender( - - Heading - - Group Heading - - , + 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).toHaveBeenCalled() spy.mockRestore() diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index 98892d2c256..c0b75c6f018 100644 --- a/packages/react/src/ActionList/Group.tsx +++ b/packages/react/src/ActionList/Group.tsx @@ -86,7 +86,7 @@ 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} @@ -112,7 +112,7 @@ export type GroupHeadingProps = Pick & { as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' - unsafeTitle?: string + internalBackwardCompatibleTitle?: string } /** @@ -127,7 +127,7 @@ export const GroupHeading: React.FC> as, variant, // We are not recommending this prop to be used, it should only be used internally for incremental rollout. - unsafeTitle, + internalBackwardCompatibleTitle, auxiliaryText, children, sx = defaultSxProp, @@ -172,14 +172,14 @@ export const GroupHeading: React.FC> {/* 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}> - {unsafeTitle ?? children} + {internalBackwardCompatibleTitle ?? children} {auxiliaryText && {auxiliaryText}} From 114cac61252f9cc98af5e2ffa1b0dcd22367cf65 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 27 Mar 2024 12:28:24 +1000 Subject: [PATCH 7/8] fix test and add another case --- .../react/src/ActionList/ActionList.test.tsx | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index 33e42c2082f..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( @@ -264,7 +293,7 @@ describe('ActionList', () => { }) 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( + expect(() => HTMLRender( Heading @@ -275,7 +304,7 @@ describe('ActionList', () => { , ), ).toThrow( - 'You are setting a heading for a list, that requires a heading level. Please use `as` prop to set a proper heading level.', + "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).toHaveBeenCalled() spy.mockRestore() From e6a19bc913c7475c3f5159f826698b3a8230886a Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 27 Mar 2024 12:32:30 +1000 Subject: [PATCH 8/8] add an underscore to the internal bc title --- packages/react/src/ActionList/Group.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index c0b75c6f018..f4c1e2e8105 100644 --- a/packages/react/src/ActionList/Group.tsx +++ b/packages/react/src/ActionList/Group.tsx @@ -86,7 +86,7 @@ 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} @@ -112,7 +112,7 @@ export type GroupHeadingProps = Pick & { as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' - internalBackwardCompatibleTitle?: string + _internalBackwardCompatibleTitle?: string } /** @@ -127,7 +127,7 @@ export const GroupHeading: React.FC> as, variant, // We are not recommending this prop to be used, it should only be used internally for incremental rollout. - internalBackwardCompatibleTitle, + _internalBackwardCompatibleTitle, auxiliaryText, children, sx = defaultSxProp, @@ -172,14 +172,14 @@ export const GroupHeading: React.FC> {/* 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}> - {internalBackwardCompatibleTitle ?? children} + {_internalBackwardCompatibleTitle ?? children} {auxiliaryText && {auxiliaryText}}