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
primer · Mar 27, 2024 · c2557d1 · c2557d1
1 parent 4b1bac1
commit c2557d1
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 23 deletions.
diff --git a/.changeset/chilly-buckets-kick.md 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
@@ -47,7 +47,7 @@ export const WithVisualListHeading = () => (
   <ActionList>
     <ActionList.Heading as="h2">Filter by</ActionList.Heading>
     <ActionList.Group>
-      <ActionList.GroupHeading>Path</ActionList.GroupHeading>
+      <ActionList.GroupHeading as="h4">Repositories</ActionList.GroupHeading>
       <ActionList.Item onClick={() => {}}>
         <ActionList.LeadingVisual>
           <FileDirectoryIcon />
@@ -75,7 +75,7 @@ export const WithVisualListHeading = () => (
     </ActionList.Group>
 
     <ActionList.Group>
-      <ActionList.GroupHeading>Advanced</ActionList.GroupHeading>
+      <ActionList.GroupHeading as="h4">Advanced</ActionList.GroupHeading>
       <ActionList.Item onClick={() => {}}>
         <ActionList.LeadingVisual>
           <PlusCircleIcon />

diff --git 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(
+        <ThemeProvider theme={theme}>
+          <SSRProvider>
+            <BaseStyles>
+              <ActionMenu open={true}>
+                <ActionMenu.Button>Trigger</ActionMenu.Button>
+                <ActionMenu.Overlay>
+                  <ActionList>
+                    <ActionList.Group>
+                      <ActionList.GroupHeading as="h2">Group Heading</ActionList.GroupHeading>
+                    </ActionList.Group>
+                  </ActionList>
+                </ActionMenu.Overlay>
+              </ActionMenu>
+            </BaseStyles>
+          </SSRProvider>
+        </ThemeProvider>,
+      ),
+    ).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(
       <ActionList>
@@ -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(
-      <ActionList>
-        <ActionList.Heading as="h1">Heading</ActionList.Heading>
-        <ActionList.Group>
-          <ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
-        </ActionList.Group>
-      </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(() =>
+      HTMLRender(
+        <ActionList>
+          <ActionList.Heading as="h1">Heading</ActionList.Heading>
+          <ActionList.Group>
+            <ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
+            <ActionList.Item>Item</ActionList.Item>
+          </ActionList.Group>
+        </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.",
     )
-    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
@@ -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. <ActionList.Group title="Group title"> → <ActionList.GroupHeading>Group title</ActionList.GroupHeading>)
    */
   title?: string
   /**
@@ -85,8 +85,10 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
     >
       <GroupContext.Provider value={{selectionVariant, groupHeadingId}}>
         {title && !slots.groupHeading ? (
-          <GroupHeading title={title} variant={variant} auxiliaryText={auxiliaryText} />
+          // Escape hatch: supports old API <ActionList.Group title="group title"> in a non breaking way
+          <GroupHeading variant={variant} auxiliaryText={auxiliaryText} _internalBackwardCompatibleTitle={title} />
         ) : null}
+        {/* Supports new API ActionList.GroupHeading */}
         {!title && slots.groupHeading ? React.cloneElement(slots.groupHeading) : null}
         <Box
           as="ul"
@@ -105,11 +107,12 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
   )
 }
 
-export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' | 'auxiliaryText'> &
+export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliaryText'> &
   Omit<ActionListHeadingProps, 'as'> &
   SxProp &
   React.HTMLAttributes<HTMLElement> & {
     as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
+    _internalBackwardCompatibleTitle?: string
   }
 
 /**
@@ -123,7 +126,8 @@ export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' |
 export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>> = ({
   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<React.PropsWithChildren<GroupHeadingProps>>
   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. <GroupHeading as="h2">...</GroupHeading>
+    !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<React.PropsWithChildren<GroupHeadingProps>>
 
   return (
     <>
-      {listRole ? (
+      {/* for listbox (SelectPanel) and menu (ActionMenu) roles, group titles are presentational. */}
+      {listRole && listRole !== 'list' ? (
         <Box sx={styles} role="presentation" aria-hidden="true" {...props}>
-          <span id={groupHeadingId}>{title ?? children}</span>
+          <span id={groupHeadingId}>{_internalBackwardCompatibleTitle ?? children}</span>
           {auxiliaryText && <span>{auxiliaryText}</span>}
         </Box>
       ) : (
+        // 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.
         <>
           <Heading as={as || 'h3'} id={groupHeadingId} sx={merge<BetterSystemStyleObject>(styles, sx)} {...props}>
-            {title ?? children}
+            {_internalBackwardCompatibleTitle ?? children}
           </Heading>
           {auxiliaryText && <span>{auxiliaryText}</span>}
         </>