Remove Box usage and sx prop from NavList and related components

.changeset/cool-clubs-think.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': major
+---
+
+Removes Box usage and sx prop from NavList and ActionList

packages/react/src/ActionList/List.tsx

@@ -1,5 +1,5 @@
 import React from 'react'
-import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
+import {fixedForwardRef} from '../utils/modern-polymorphic'
 import {ActionListContainerContext} from './ActionListContainerContext'
 import {useSlots} from '../hooks/useSlots'
 import {Heading} from './Heading'
@@ -11,68 +11,80 @@ import {clsx} from 'clsx'
 import classes from './ActionList.module.css'
 import {BoxWithFallback} from '../internal/components/BoxWithFallback'
 
-export const List = React.forwardRef<HTMLUListElement, ActionListProps>(
-  (
-    {variant = 'inset', selectionVariant, showDividers = false, role, disableFocusZone = false, className, ...props},
-    forwardedRef,
-  ): JSX.Element => {
-    const [slots, childrenWithoutSlots] = useSlots(props.children, {
-      heading: Heading,
-    })
+const UnwrappedList = <As extends React.ElementType = 'ul'>(
+  props: ActionListProps<As>,
+  forwardedRef: React.Ref<unknown>,
+): JSX.Element => {
+  const {
+    as: Component = 'ul',
+    variant = 'inset',
+    selectionVariant,
+    showDividers = false,
+    role,
+    disableFocusZone = false,
+    className,
+    ...restProps
+  } = props
+  const [slots, childrenWithoutSlots] = useSlots(restProps.children, {
+    heading: Heading,
+  })
 
-    const headingId = useId()
+  const headingId = useId()
 
-    /** if list is inside a Menu, it will get a role from the Menu */
-    const {
-      listRole: listRoleFromContainer,
-      listLabelledBy,
-      selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation
-      enableFocusZone: enableFocusZoneFromContainer,
-      container,
-    } = React.useContext(ActionListContainerContext)
+  /** if list is inside a Menu, it will get a role from the Menu */
+  const {
+    listRole: listRoleFromContainer,
+    listLabelledBy,
+    selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation
+    enableFocusZone: enableFocusZoneFromContainer,
+    container,
+  } = React.useContext(ActionListContainerContext)
 
-    const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy
-    const listRole = role || listRoleFromContainer
-    const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject<HTMLUListElement>)
+  const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy
+  const listRole = role || listRoleFromContainer
+  const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject<HTMLUListElement>)
 
-    let enableFocusZone = false
-    if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer
-    else if (listRole && !disableFocusZone) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole)
+  let enableFocusZone = false
+  if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer
+  else if (listRole && !disableFocusZone) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole)
 
-    useFocusZone({
-      disabled: !enableFocusZone,
-      containerRef: listRef,
-      bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown,
-      focusOutBehavior:
-        listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined,
-    })
+  useFocusZone({
+    disabled: !enableFocusZone,
+    containerRef: listRef,
+    bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown,
+    focusOutBehavior:
+      listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined,
+  })
 
-    return (
-      <ListContext.Provider
-        value={{
-          variant,
-          selectionVariant: selectionVariant || containerSelectionVariant,
-          showDividers,
-          role: listRole,
-          headingId,
-        }}
+  return (
+    <ListContext.Provider
+      value={{
+        variant,
+        selectionVariant: selectionVariant || containerSelectionVariant,
+        showDividers,
+        role: listRole,
+        headingId,
+      }}
+    >
+      {slots.heading}
+      <BoxWithFallback
+        as={Component}
+        className={clsx(classes.ActionList, className)}
+        role={listRole}
+        aria-labelledby={ariaLabelledBy}
+        ref={listRef}
+        data-dividers={showDividers}
+        data-variant={variant}
+        {...restProps}
       >
-        {slots.heading}
-        <BoxWithFallback
-          as="ul"
-          className={clsx(classes.ActionList, className)}
-          role={listRole}
-          aria-labelledby={ariaLabelledBy}
-          ref={listRef}
-          data-dividers={showDividers}
-          data-variant={variant}
-          {...props}
-        >
-          {childrenWithoutSlots}
-        </BoxWithFallback>
-      </ListContext.Provider>
-    )
-  },
-) as PolymorphicForwardRefComponent<'ul', ActionListProps>
+        {childrenWithoutSlots}
+      </BoxWithFallback>
+    </ListContext.Provider>
+  )
+}
 
-List.displayName = 'ActionList'
+const List = fixedForwardRef(UnwrappedList)
+
+Object.assign(List, {displayName: 'ActionList'})
+
+export {List}

packages/react/src/ActionList/shared.ts

@@ -1,8 +1,15 @@
 import React from 'react'
 import type {SxProp} from '../sx'
 import type {AriaRole} from '../utils/types'
+import type {PolymorphicProps} from '../utils/modern-polymorphic'
 
-export type ActionListItemProps = {
+// need to explicitly omit `onSelect` from native HTML <li> attributes to avoid collision
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type ExcludeSelectEventHandler<T> = T extends any ? Omit<T, 'onSelect'> : never
+
+export type ActionListItemProps<As extends React.ElementType = 'li'> = ExcludeSelectEventHandler<
+  PolymorphicProps<As, 'li'>
+> & {
   /**
    * Primary content for an Item
    */
@@ -57,6 +64,10 @@ export type ActionListItemProps = {
   groupId?: string
   renderItem?: (item: React.FC<React.PropsWithChildren<MenuItemProps>>) => React.ReactNode
   handleAddItem?: (item: React.FC<React.PropsWithChildren<MenuItemProps>>) => void
+  /**
+   * @deprecated `as` prop has no effect on `ActionList.Item`, only `ActionList.LinkItem`
+   */
+  as?: As
 } & SxProp
 
 type MenuItemProps = {
@@ -70,7 +81,7 @@ type MenuItemProps = {
   className?: string
 }
 
-export type ItemContext = Pick<ActionListItemProps, 'variant' | 'disabled' | 'size'> & {
+export type ItemContext = Pick<ActionListItemProps<React.ElementType>, 'variant' | 'disabled' | 'size'> & {
   inlineDescriptionId?: string
   blockDescriptionId?: string
   trailingVisualId?: string
@@ -80,8 +91,8 @@ export type ItemContext = Pick<ActionListItemProps, 'variant' | 'disabled' | 'si
 export const ItemContext = React.createContext<ItemContext>({})
 
 export const getVariantStyles = (
-  variant: ActionListItemProps['variant'],
-  disabled: ActionListItemProps['disabled'],
+  variant: ActionListItemProps<React.ElementType>['variant'],
+  disabled: ActionListItemProps<React.ElementType>['disabled'],
   inactive?: boolean,
 ) => {
   if (disabled) {
@@ -120,32 +131,39 @@ export const getVariantStyles = (
 
 export const TEXT_ROW_HEIGHT = '20px' // custom value off the scale
 
-export type ActionListProps = React.PropsWithChildren<{
-  /**
-   * `inset` children are offset (vertically and horizontally) from `List`’s edges, `full` children are flush (vertically and horizontally) with `List` edges
-   */
-  variant?: 'inset' | 'horizontal-inset' | 'full'
-  /**
-   * Whether multiple Items or a single Item can be selected.
-   */
-  selectionVariant?: 'single' | 'radio' | 'multiple'
-  /**
-   * Display a divider above each `Item` in this `List` when it does not follow a `Header` or `Divider`.
-   */
-  showDividers?: boolean
-  /**
-   * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values.
-   */
-  role?: AriaRole
-  /**
-   * Disables the focus zone for the list if applicable. Focus zone is enabled by default for `menu` and `listbox` roles, or components such as `ActionMenu` and `SelectPanel`.
-   */
-  disableFocusZone?: boolean
-  className?: string
-}> &
+export type ActionListProps<As extends React.ElementType = 'ul'> = PolymorphicProps<
+  As,
+  'ul',
+  React.PropsWithChildren<{
+    /**
+     * `inset` children are offset (vertically and horizontally) from `List`’s edges, `full` children are flush (vertically and horizontally) with `List` edges
+     */
+    variant?: 'inset' | 'horizontal-inset' | 'full'
+    /**
+     * Whether multiple Items or a single Item can be selected.
+     */
+    selectionVariant?: 'single' | 'radio' | 'multiple'
+    /**
+     * Display a divider above each `Item` in this `List` when it does not follow a `Header` or `Divider`.
+     */
+    showDividers?: boolean
+    /**
+     * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values.
+     */
+    role?: AriaRole
+    /**
+     * Disables the focus zone for the list if applicable. Focus zone is enabled by default for `menu` and `listbox` roles, or components such as `ActionMenu` and `SelectPanel`.
+     */
+    disableFocusZone?: boolean
+    className?: string
+  }>
+> &
   SxProp
 
-type ContextProps = Pick<ActionListProps, 'variant' | 'selectionVariant' | 'showDividers' | 'role'> & {
+type ContextProps = Pick<
+  ActionListProps<React.ElementType>,
+  'variant' | 'selectionVariant' | 'showDividers' | 'role'
+> & {
   headingId?: string
 }
 

packages/react/src/Autocomplete/AutocompleteMenu.tsx

@@ -6,7 +6,7 @@ import type {ScrollIntoViewOptions} from '@primer/behaviors'
 import type {ActionListItemProps} from '../ActionList'
 import {ActionList} from '../ActionList'
 import {useFocusZone} from '../hooks/useFocusZone'
-import type {ComponentProps, MandateProps} from '../utils/types'
+import type {ComponentProps, MandateProps, AriaRole} from '../utils/types'
 import Spinner from '../Spinner'
 import {useId} from '../hooks/useId'
 import {AutocompleteContext} from './AutocompleteContext'
@@ -365,19 +365,25 @@ function AutocompleteMenu<T extends AutocompleteItemProps>(props: AutocompleteMe
                   text,
                   leadingVisual: LeadingVisual,
                   trailingVisual: TrailingVisual,
-                  // @ts-expect-error this is defined in the items above but is
-                  // missing in TS
                   key,
+                  role,
                   ...itemProps
                 } = item
                 return (
-                  <ActionList.Item key={key ?? id} onSelect={() => onAction(item)} {...itemProps} id={id} data-id={id}>
+                  <ActionList.Item
+                    key={(key ?? id) as string | number}
+                    onSelect={() => onAction(item)}
+                    {...itemProps}
+                    id={id}
+                    data-id={id}
+                    role={role as AriaRole}
+                  >
                     {LeadingVisual && (
                       <ActionList.LeadingVisual>
                         {isElement(LeadingVisual) ? LeadingVisual : <LeadingVisual />}
                       </ActionList.LeadingVisual>
                     )}
-                    {children ?? text}
+                    {(children ?? text) as React.ReactNode}
                     {TrailingVisual && (
                       <ActionList.TrailingVisual>
                         {isElement(TrailingVisual) ? TrailingVisual : <TrailingVisual />}