Merge 9b21aa0 into 3be64c5

.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

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

@@ -46,7 +46,8 @@ export const SimpleList = () => (
 export const WithVisualListHeading = () => (
   <ActionList>
     <ActionList.Heading as="h2">Filter by</ActionList.Heading>
-    <ActionList.Group title="Path">
+    <ActionList.Group>
+      <ActionList.GroupHeading as="h4">Repositories</ActionList.GroupHeading>
       <ActionList.Item onClick={() => {}}>
         <ActionList.LeadingVisual>
           <FileDirectoryIcon />
@@ -73,7 +74,8 @@ export const WithVisualListHeading = () => (
       </ActionList.Item>
     </ActionList.Group>
 
-    <ActionList.Group title="Advanced">
+    <ActionList.Group>
+      <ActionList.GroupHeading as="h4">Advanced</ActionList.GroupHeading>
       <ActionList.Item onClick={() => {}}>
         <ActionList.LeadingVisual>
           <PlusCircleIcon />

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(
       <ActionList>
@@ -273,7 +273,7 @@ describe('ActionList', () => {
         </ActionList.Group>
       </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 () => {

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} unsafeTitle={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'
+    unsafeTitle?: 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.
+  unsafeTitle,
   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}>{unsafeTitle ?? 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}
+            {unsafeTitle ?? children}
           </Heading>
           {auxiliaryText && <span>{auxiliaryText}</span>}
         </>