diff --git a/.changeset/cool-clubs-think.md b/.changeset/cool-clubs-think.md new file mode 100644 index 00000000000..f025ef30476 --- /dev/null +++ b/.changeset/cool-clubs-think.md @@ -0,0 +1,5 @@ +--- +'@primer/react': major +--- + +Removes Box usage and sx prop from NavList and ActionList diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 49944d74fe2..65408b623b4 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -2,7 +2,6 @@ import React from 'react' import {useId} from '../hooks/useId' import {useSlots} from '../hooks/useSlots' -import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {ActionListContainerContext} from './ActionListContainerContext' import {Description} from './Description' import {GroupContext} from './Group' @@ -17,6 +16,7 @@ import VisuallyHidden from '../_VisuallyHidden' import classes from './ActionList.module.css' import {clsx} from 'clsx' import {BoxWithFallback} from '../internal/components/BoxWithFallback' +import {fixedForwardRef} from '../utils/modern-polymorphic' type ActionListSubItemProps = { children?: React.ReactNode @@ -28,285 +28,298 @@ export const SubItem: React.FC = ({children}) => { SubItem.displayName = 'ActionList.SubItem' -const ButtonItemContainerNoBox = React.forwardRef(({children, style, ...props}, forwardedRef) => { - return ( - - ) -}) as PolymorphicForwardRefComponent - -const DivItemContainerNoBox = React.forwardRef(({children, ...props}, forwardedRef) => { - return ( -
} {...props}> - {children} -
- ) -}) as PolymorphicForwardRefComponent - -export const Item = React.forwardRef( - ( - { - variant = 'default', - size = 'medium', - disabled = false, - inactiveText, - selected = undefined, - active = false, - onSelect: onSelectUser, - sx: sxProp, - id, - role, - loading, - _PrivateItemWrapper, - className, - groupId: _groupId, - renderItem: _renderItem, - handleAddItem: _handleAddItem, - ...props - }, - forwardedRef, - ): JSX.Element => { - const baseSlots = { - leadingVisual: LeadingVisual, - trailingVisual: TrailingVisual, - trailingAction: TrailingAction, - subItem: SubItem, - } - - const [partialSlots, childrenWithoutSlots] = useSlots(props.children, {...baseSlots, description: Description}) - - const slots = {description: undefined, ...partialSlots} - - const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = - React.useContext(ActionListContainerContext) - - // Be sure to avoid rendering the container unless there is a default - const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( - {defaultTrailingVisual} - ) : null - const trailingVisual = slots.trailingVisual ?? wrappedDefaultTrailingVisual - - const {role: listRole, selectionVariant: listSelectionVariant} = React.useContext(ListContext) - const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext) - const inactive = Boolean(inactiveText) - // TODO change `menuContext` check to ```listRole !== undefined && ['menu', 'listbox'].includes(listRole)``` - // once we have a better way to handle existing usage in dotcom that incorrectly use ActionList.TrailingAction - const menuContext = container === 'ActionMenu' || container === 'SelectPanel' || container === 'FilteredActionList' - // TODO: when we change `menuContext` to check `listRole` instead of `container` - const showInactiveIndicator = inactive && !(listRole !== undefined && ['menu', 'listbox'].includes(listRole)) - - const onSelect = React.useCallback( - ( - event: React.MouseEvent | React.KeyboardEvent, - // eslint-disable-next-line @typescript-eslint/no-unsafe-function-type - afterSelect?: Function, - ) => { - if (typeof onSelectUser === 'function') onSelectUser(event) - if (event.defaultPrevented) return - if (typeof afterSelect === 'function') afterSelect(event) - }, - [onSelectUser], +const ButtonItemContainerNoBox = React.forwardRef>( + ({children, style, ...props}, forwardedRef) => { + return ( + ) + }, +) - const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant - ? groupSelectionVariant - : listSelectionVariant - - /** Infer item role based on the container */ - let inferredItemRole: ActionListItemProps['role'] - if (container === 'ActionMenu') { - if (selectionVariant === 'single') inferredItemRole = 'menuitemradio' - else if (selectionVariant === 'multiple') inferredItemRole = 'menuitemcheckbox' - else inferredItemRole = 'menuitem' - } else if (listRole === 'listbox') { - if (selectionVariant !== undefined && !role) inferredItemRole = 'option' - } - - const itemRole = role || inferredItemRole - - if (slots.trailingAction) { - invariant( - !menuContext, - `ActionList.TrailingAction can not be used within a list with an ARIA role of "menu" or "listbox".`, - ) - } - - /** Infer the proper selection attribute based on the item's role */ - let inferredSelectionAttribute: 'aria-selected' | 'aria-checked' | undefined - if (itemRole === 'menuitemradio' || itemRole === 'menuitemcheckbox') inferredSelectionAttribute = 'aria-checked' - else if (itemRole === 'option') inferredSelectionAttribute = 'aria-selected' - - const itemSelectionAttribute = selectionAttribute || inferredSelectionAttribute - // Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive - const listItemSemantics = - role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox' - - const listRoleTypes = ['listbox', 'menu', 'list'] - const listSemantics = (listRole && listRoleTypes.includes(listRole)) || inactive || listItemSemantics - const buttonSemantics = !listSemantics && !_PrivateItemWrapper - - const clickHandler = React.useCallback( - (event: React.MouseEvent) => { - if (disabled || inactive || loading) return - onSelect(event, afterSelect) - }, - [onSelect, disabled, inactive, afterSelect, loading], +const DivItemContainerNoBox = React.forwardRef>( + ({children, ...props}, forwardedRef) => { + return ( +
} {...props}> + {children} +
) + }, +) + +const UnwrappedItem = ( + { + variant = 'default', + size = 'medium', + disabled = false, + inactiveText, + selected = undefined, + active = false, + onSelect: onSelectUser, + sx: sxProp, + id, + role, + loading, + _PrivateItemWrapper, + className, + groupId: _groupId, + renderItem: _renderItem, + handleAddItem: _handleAddItem, + ...props + }: ActionListItemProps, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + forwardedRef: React.Ref, +): JSX.Element => { + const baseSlots = { + leadingVisual: LeadingVisual, + trailingVisual: TrailingVisual, + trailingAction: TrailingAction, + subItem: SubItem, + } + + const [partialSlots, childrenWithoutSlots] = useSlots(props.children, {...baseSlots, description: Description}) + + const slots = {description: undefined, ...partialSlots} + + const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = + React.useContext(ActionListContainerContext) + + // Be sure to avoid rendering the container unless there is a default + const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( + {defaultTrailingVisual} + ) : null + const trailingVisual = slots.trailingVisual ?? wrappedDefaultTrailingVisual + + const {role: listRole, selectionVariant: listSelectionVariant} = React.useContext(ListContext) + const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext) + const inactive = Boolean(inactiveText) + // TODO change `menuContext` check to ```listRole !== undefined && ['menu', 'listbox'].includes(listRole)``` + // once we have a better way to handle existing usage in dotcom that incorrectly use ActionList.TrailingAction + const menuContext = container === 'ActionMenu' || container === 'SelectPanel' || container === 'FilteredActionList' + // TODO: when we change `menuContext` to check `listRole` instead of `container` + const showInactiveIndicator = inactive && !(listRole !== undefined && ['menu', 'listbox'].includes(listRole)) + + const onSelect = React.useCallback( + ( + event: React.MouseEvent | React.KeyboardEvent, + // eslint-disable-next-line @typescript-eslint/no-unsafe-function-type + afterSelect?: Function, + ) => { + if (typeof onSelectUser === 'function') onSelectUser(event) + if (event.defaultPrevented) return + if (typeof afterSelect === 'function') afterSelect(event) + }, + [onSelectUser], + ) - const keyPressHandler = React.useCallback( - (event: React.KeyboardEvent) => { - if (disabled || inactive || loading) return - if ([' ', 'Enter'].includes(event.key)) { - if (event.key === ' ') { - event.preventDefault() // prevent scrolling on Space - // immediately reset defaultPrevented once its job is done - // so as to not disturb the functions that use that event after this - event.defaultPrevented = false - } - onSelect(event, afterSelect) - } - }, - [onSelect, disabled, loading, inactive, afterSelect], + const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant + ? groupSelectionVariant + : listSelectionVariant + + /** Infer item role based on the container */ + let inferredItemRole: ActionListItemProps['role'] + if (container === 'ActionMenu') { + if (selectionVariant === 'single') inferredItemRole = 'menuitemradio' + else if (selectionVariant === 'multiple') inferredItemRole = 'menuitemcheckbox' + else inferredItemRole = 'menuitem' + } else if (listRole === 'listbox') { + if (selectionVariant !== undefined && !role) inferredItemRole = 'option' + } + + const itemRole = role || inferredItemRole + + if (slots.trailingAction) { + invariant( + !menuContext, + `ActionList.TrailingAction can not be used within a list with an ARIA role of "menu" or "listbox".`, ) + } + + /** Infer the proper selection attribute based on the item's role */ + let inferredSelectionAttribute: 'aria-selected' | 'aria-checked' | undefined + if (itemRole === 'menuitemradio' || itemRole === 'menuitemcheckbox') inferredSelectionAttribute = 'aria-checked' + else if (itemRole === 'option') inferredSelectionAttribute = 'aria-selected' + + const itemSelectionAttribute = selectionAttribute || inferredSelectionAttribute + // Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive + const listItemSemantics = + role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox' + + const listRoleTypes = ['listbox', 'menu', 'list'] + const listSemantics = (listRole && listRoleTypes.includes(listRole)) || inactive || listItemSemantics + const buttonSemantics = !listSemantics && !_PrivateItemWrapper + + const clickHandler = React.useCallback( + (event: React.MouseEvent) => { + if (disabled || inactive || loading) return + onSelect(event, afterSelect) + }, + [onSelect, disabled, inactive, afterSelect, loading], + ) - const itemId = useId(id) - const labelId = `${itemId}--label` - const inlineDescriptionId = `${itemId}--inline-description` - const blockDescriptionId = `${itemId}--block-description` - const trailingVisualId = `${itemId}--trailing-visual` - const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined - - const DefaultItemWrapper = listSemantics ? DivItemContainerNoBox : ButtonItemContainerNoBox - - const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper - - // only apply aria-selected and aria-checked to selectable items - const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option'] - const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole) - - let focusable - - if (showInactiveIndicator) { - focusable = true - } - - // Extract the variant prop value from the description slot component - const descriptionVariant = slots.description?.props.variant ?? 'inline' - - const menuItemProps = { - onClick: clickHandler, - onKeyPress: !buttonSemantics ? keyPressHandler : undefined, - 'aria-disabled': disabled ? true : undefined, - 'data-inactive': inactive ? true : undefined, - 'data-loading': loading && !inactive ? true : undefined, - tabIndex: focusable ? undefined : 0, - 'aria-labelledby': `${labelId} ${slots.trailingVisual ? trailingVisualId : ''} ${ - slots.description && descriptionVariant === 'inline' ? inlineDescriptionId : '' - }`, - 'aria-describedby': - [ - slots.description && descriptionVariant === 'block' ? blockDescriptionId : undefined, - inactiveWarningId ?? undefined, - ] - .filter(String) - .join(' ') - .trim() || undefined, - ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), - role: itemRole, - id: itemId, - } - - const containerProps = _PrivateItemWrapper - ? {role: itemRole ? 'none' : undefined, ...props} - : // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - (listSemantics && {...menuItemProps, ...props, ref: forwardedRef}) || {} - - const wrapperProps = _PrivateItemWrapper - ? menuItemProps - : !listSemantics && { - ...menuItemProps, - ...props, - ref: forwardedRef, + const keyPressHandler = React.useCallback( + (event: React.KeyboardEvent) => { + if (disabled || inactive || loading) return + if ([' ', 'Enter'].includes(event.key)) { + if (event.key === ' ') { + event.preventDefault() // prevent scrolling on Space + // immediately reset defaultPrevented once its job is done + // so as to not disturb the functions that use that event after this + event.defaultPrevented = false } + onSelect(event, afterSelect) + } + }, + [onSelect, disabled, loading, inactive, afterSelect], + ) - return ( - + - - - - + + + + {slots.leadingVisual} + + + + + {childrenWithoutSlots} + {/* Loading message needs to be in here so it is read with the label */} + {/* If the item is inactive, we do not simultaneously announce that it is loading */} + {loading === true && !inactive && Loading} + + {slots.description} + - {slots.leadingVisual} + {trailingVisual} - - - - {childrenWithoutSlots} - {/* Loading message needs to be in here so it is read with the label */} - {/* If the item is inactive, we do not simultaneously announce that it is loading */} - {loading === true && !inactive && Loading} + + { + // If the item is inactive, but it's not in an overlay (e.g. ActionMenu, SelectPanel), + // render the inactive warning message directly in the item. + !showInactiveIndicator && inactiveText ? ( + + {inactiveText} - {slots.description} - - - {trailingVisual} - - - { - // If the item is inactive, but it's not in an overlay (e.g. ActionMenu, SelectPanel), - // render the inactive warning message directly in the item. - !showInactiveIndicator && inactiveText ? ( - - {inactiveText} - - ) : null - } - - - {!inactive && !loading && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction} - {slots.subItem} - - - ) - }, -) as PolymorphicForwardRefComponent<'li', ActionListItemProps> + ) : null + } + + + {!inactive && !loading && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction} + {slots.subItem} + + + ) +} + +const Item = fixedForwardRef(UnwrappedItem) + +Object.assign(Item, {displayName: 'ActionList.Item'}) -Item.displayName = 'ActionList.Item' +export {Item} diff --git a/packages/react/src/ActionList/List.tsx b/packages/react/src/ActionList/List.tsx index f8448ba0055..97ead828fea 100644 --- a/packages/react/src/ActionList/List.tsx +++ b/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( - ( - {variant = 'inset', selectionVariant, showDividers = false, role, disableFocusZone = false, className, ...props}, - forwardedRef, - ): JSX.Element => { - const [slots, childrenWithoutSlots] = useSlots(props.children, { - heading: Heading, - }) +const UnwrappedList = ( + props: ActionListProps, + forwardedRef: React.Ref, +): 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) + const ariaLabelledBy = slots.heading ? (slots.heading.props.id ?? headingId) : listLabelledBy + const listRole = role || listRoleFromContainer + const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject) - 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 ( - + {slots.heading} + - {slots.heading} - - {childrenWithoutSlots} - - - ) - }, -) as PolymorphicForwardRefComponent<'ul', ActionListProps> + {childrenWithoutSlots} + + + ) +} -List.displayName = 'ActionList' +const List = fixedForwardRef(UnwrappedList) + +Object.assign(List, {displayName: 'ActionList'}) + +export {List} diff --git a/packages/react/src/ActionList/shared.ts b/packages/react/src/ActionList/shared.ts index a4563ee4f7b..fe3d937c5f8 100644 --- a/packages/react/src/ActionList/shared.ts +++ b/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
  • attributes to avoid collision +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type ExcludeSelectEventHandler = T extends any ? Omit : never + +export type ActionListItemProps = ExcludeSelectEventHandler< + PolymorphicProps +> & { /** * Primary content for an Item */ @@ -57,6 +64,10 @@ export type ActionListItemProps = { groupId?: string renderItem?: (item: React.FC>) => React.ReactNode handleAddItem?: (item: React.FC>) => 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 & { +export type ItemContext = Pick, 'variant' | 'disabled' | 'size'> & { inlineDescriptionId?: string blockDescriptionId?: string trailingVisualId?: string @@ -80,8 +91,8 @@ export type ItemContext = Pick({}) export const getVariantStyles = ( - variant: ActionListItemProps['variant'], - disabled: ActionListItemProps['disabled'], + variant: ActionListItemProps['variant'], + disabled: ActionListItemProps['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 = 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 & { +type ContextProps = Pick< + ActionListProps, + 'variant' | 'selectionVariant' | 'showDividers' | 'role' +> & { headingId?: string } diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index bc00c27eb64..d2f19b6d6f7 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/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(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 ( - onAction(item)} {...itemProps} id={id} data-id={id}> + onAction(item)} + {...itemProps} + id={id} + data-id={id} + role={role as AriaRole} + > {LeadingVisual && ( {isElement(LeadingVisual) ? LeadingVisual : } )} - {children ?? text} + {(children ?? text) as React.ReactNode} {TrailingVisual && ( {isElement(TrailingVisual) ? TrailingVisual : } diff --git a/packages/react/src/NavList/NavList.docs.json b/packages/react/src/NavList/NavList.docs.json index e3315ab91ab..d1800f44558 100644 --- a/packages/react/src/NavList/NavList.docs.json +++ b/packages/react/src/NavList/NavList.docs.json @@ -26,11 +26,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"nav\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "subcomponents": [ @@ -68,11 +63,6 @@ "name": "as", "type": "React.ElementType", "defaultValue": "\"a\"" - }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true } ], "passthrough": { @@ -83,11 +73,6 @@ { "name": "NavList.LeadingVisual", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -97,11 +82,6 @@ { "name": "NavList.TrailingVisual", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -111,11 +91,6 @@ { "name": "NavList.SubNav", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -137,11 +112,6 @@ "type": "string", "description": "The text that gets rendered as the group's heading. Alternatively, you can pass the `NavList.GroupHeading` component as a child of `NavList.Group`.\nIf both `title` and `NavList.GroupHeading` are passed, `NavList.GroupHeading` will be rendered as the heading." }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -172,11 +142,6 @@ "description": "", "defaultValue": "" }, - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -186,11 +151,6 @@ { "name": "NavList.Divider", "props": [ - { - "name": "sx", - "type": "SystemStyleObject", - "deprecated": true - }, { "name": "ref", "type": "React.RefObject" @@ -264,4 +224,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx index 360558fded8..282bdae1e75 100644 --- a/packages/react/src/NavList/NavList.tsx +++ b/packages/react/src/NavList/NavList.tsx @@ -1,6 +1,7 @@ import {ChevronDownIcon, PlusIcon, type Icon} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import React, {isValidElement} from 'react' +import {clsx} from 'clsx' import type { ActionListTrailingActionProps, ActionListDividerProps, @@ -11,27 +12,22 @@ import type { import {ActionList} from '../ActionList' import {SubItem} from '../ActionList/Item' import {ActionListContainerContext} from '../ActionList/ActionListContainerContext' -import Box from '../Box' -import type {SxProp} from '../sx' -import {merge} from '../sx' -import {defaultSxProp} from '../utils/defaultSxProp' import {useId} from '../hooks/useId' import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' import classes from '../ActionList/ActionList.module.css' +import navListClasses from './NavList.module.css' import {flushSync} from 'react-dom' -import {BoxWithFallback} from '../internal/components/BoxWithFallback' // ---------------------------------------------------------------------------- // NavList export type NavListProps = { children: React.ReactNode -} & SxProp & - React.ComponentProps<'nav'> +} & React.ComponentProps<'nav'> const Root = React.forwardRef(({children, ...props}, ref) => { return ( - + ) }) @@ -54,10 +50,10 @@ export type NavListItemProps = { href?: string 'aria-current'?: 'page' | 'step' | 'location' | 'date' | 'time' | 'true' | 'false' | boolean inactiveText?: string -} & SxProp +} const Item = React.forwardRef( - ({'aria-current': ariaCurrent, children, defaultOpen, sx: sxProp = defaultSxProp, ...props}, ref) => { + ({'aria-current': ariaCurrent, children, defaultOpen, ...props}, ref) => { const {depth} = React.useContext(SubNavContext) // Get SubNav from children @@ -79,7 +75,6 @@ const Item = React.forwardRef( subNav={subNav} depth={depth} defaultOpen={defaultOpen} - sx={sxProp} style={{'--subitem-depth': depth} as React.CSSProperties} > {childrenWithoutSubNavOrTrailingAction} @@ -112,7 +107,7 @@ type ItemWithSubNavProps = { depth: number defaultOpen?: boolean style: React.CSSProperties -} & SxProp +} const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string; isOpen: boolean}>({ buttonId: '', @@ -120,14 +115,7 @@ const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: s isOpen: false, }) -function ItemWithSubNav({ - children, - subNav, - depth: _depth, - defaultOpen, - style, - sx: sxProp = defaultSxProp, -}: ItemWithSubNavProps) { +function ItemWithSubNav({children, subNav, depth: _depth, defaultOpen, style}: ItemWithSubNavProps) { const buttonId = useId() const subNavId = useId() const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? false) @@ -147,28 +135,6 @@ function ItemWithSubNav({ } }, [subNav, buttonId]) - if (sxProp !== defaultSxProp) { - return ( - - setIsOpen(open => !open)} - style={style} - sx={sxProp} - > - {children} - {/* What happens if the user provides a TrailingVisual? */} - - - - {React.cloneElement(subNav as React.ReactElement, {ref: subNavRef})} - - - ) - } return ( ({depth: 0}) // NOTE: SubNav must be a direct child of an Item -const SubNav = React.forwardRef(({children, sx: sxProp = defaultSxProp}: NavListSubNavProps, forwardedRef) => { +const SubNav = React.forwardRef(({children}, forwardedRef) => { const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext) const {depth} = React.useContext(SubNavContext) if (!buttonId || !subNavId) { @@ -215,22 +181,6 @@ const SubNav = React.forwardRef(({children, sx: sxProp = defaultSxProp}: NavList return null } - if (sxProp !== defaultSxProp) { - return ( - - - {children} - - - ) - } return (
      @@ -283,18 +233,9 @@ TrailingAction.displayName = 'NavList.TrailingAction' export type NavListGroupProps = React.HTMLAttributes & { children: React.ReactNode title?: string -} & SxProp +} -const defaultSx = {} -const Group: React.FC = ({title, children, sx: sxProp = defaultSx, ...props}) => { - if (sxProp !== defaultSx) { - return ( - - {title ? {title} : null} - {children} - - ) - } +const Group: React.FC = ({title, children, ...props}) => { return ( <> @@ -419,20 +360,11 @@ export type NavListGroupHeadingProps = ActionListGroupHeadingProps * This is an alternative to the `title` prop on `NavList.Group`. * It was primarily added to allow links in group headings. */ -const GroupHeading: React.FC = ({as = 'h3', sx: sxProp = defaultSxProp, ...rest}) => { +const GroupHeading: React.FC = ({as = 'h3', className, ...rest}) => { return ( ( - { - '> a {': { - color: 'var(--fgColor-default)', - textDecoration: 'inherit', - ':hover': {textDecoration: 'underline'}, - }, - }, - sxProp, - )} + className={clsx(navListClasses.GroupHeading, className)} data-component="NavList.GroupHeading" headingWrapElement="li" {...rest} diff --git a/packages/react/src/SegmentedControl/SegmentedControl.tsx b/packages/react/src/SegmentedControl/SegmentedControl.tsx index f0e3ce38fd0..a706f41a0ef 100644 --- a/packages/react/src/SegmentedControl/SegmentedControl.tsx +++ b/packages/react/src/SegmentedControl/SegmentedControl.tsx @@ -132,7 +132,7 @@ const Root: React.FC> = ({ { + onSelect={event => { isUncontrolled && setSelectedIndexInternalState(index) onChange && onChange(index) child.props.onClick && child.props.onClick(event as React.MouseEvent) diff --git a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap index 574c3bf2a19..d2483dd002d 100644 --- a/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap +++ b/packages/react/src/__tests__/__snapshots__/exports.test.ts.snap @@ -95,6 +95,7 @@ exports[`@primer/react > should not update exports without a semver change 1`] = "merge", "NavList", "type NavListDividerProps", + "type NavListGroupHeadingProps", "type NavListGroupProps", "type NavListItemProps", "type NavListLeadingVisualProps", diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index c64013bba6c..65a4f2492b0 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -117,6 +117,7 @@ export type { NavListLeadingVisualProps, NavListTrailingVisualProps, NavListDividerProps, + NavListGroupHeadingProps, } from './NavList' export {default as Overlay} from './Overlay' export type {OverlayProps} from './Overlay' diff --git a/packages/react/src/utils/modern-polymorphic.ts b/packages/react/src/utils/modern-polymorphic.ts index 5f83844ac10..d29e9e14f24 100644 --- a/packages/react/src/utils/modern-polymorphic.ts +++ b/packages/react/src/utils/modern-polymorphic.ts @@ -13,7 +13,7 @@ type DistributiveOmit = T extends unknown ? Omi */ // TODO: figure out how to change this type so we can set displayName // like this: `ComponentName.displayName = 'DisplayName' instead of using workarounds -type FixedForwardRef = = Record>( +type FixedForwardRef = ( render: (props: P, ref: React.Ref) => React.ReactNode, ) => (props: P & React.RefAttributes) => React.ReactNode @@ -29,7 +29,7 @@ export const fixedForwardRef = forwardRef as FixedForwardRef export type PolymorphicProps< TAs extends ElementType, TDefaultElement extends ElementType = 'div', - Props extends Record = Record, + Props = object, > = DistributiveOmit & Props, 'as'> & { as?: TAs } diff --git a/packages/styled-react/src/__tests__/primer-react.browser.test.tsx b/packages/styled-react/src/__tests__/primer-react.browser.test.tsx index f86970ef32f..ef1f7b70823 100644 --- a/packages/styled-react/src/__tests__/primer-react.browser.test.tsx +++ b/packages/styled-react/src/__tests__/primer-react.browser.test.tsx @@ -235,7 +235,7 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test.todo('NavList.Item supports `sx` prop', () => { + test('NavList.Item supports `sx` prop', () => { render( @@ -243,12 +243,36 @@ describe('@primer/react', () => { , ) - expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') + + const itemAnchorEl = screen.getByTestId('component') + const itemLiEl = itemAnchorEl.closest('li') + expect(itemLiEl).not.toBeNull() + expect(window.getComputedStyle(itemLiEl!).backgroundColor).toBe('rgb(255, 0, 0)') }) test('NavList.Group supports `sx` prop', () => { - const {container} = render(test) - expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)') + render( + + + item + + , + ) + expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') + }) + + test('NavList.GroupHeading supports `sx` prop', () => { + render( + + + + test + + item + + , + ) + expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) test('NavList.LeadingVisual supports `sx` prop', () => { @@ -375,12 +399,12 @@ describe('@primer/react', () => { expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test('SubNav supports `sx` prop', () => { + test.skip('SubNav supports `sx` prop', () => { render() expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) - test('SubNav.Link supports `sx` prop', () => { + test.skip('SubNav.Link supports `sx` prop', () => { render() expect(window.getComputedStyle(screen.getByTestId('component')).backgroundColor).toBe('rgb(255, 0, 0)') }) diff --git a/packages/styled-react/src/components/NavList.tsx b/packages/styled-react/src/components/NavList.tsx new file mode 100644 index 00000000000..2519af4870e --- /dev/null +++ b/packages/styled-react/src/components/NavList.tsx @@ -0,0 +1,56 @@ +import {NavList as PrimerNavList, Box} from '@primer/react' +import type { + NavListProps as PrimerNavListProps, + NavListItemProps as PrimerNavListItemProps, + NavListGroupProps as PrimerNavListGroupProps, +} from '@primer/react' +import {forwardRef, type ComponentProps, type PropsWithChildren} from 'react' +import {type SxProp} from '../sx' + +type NavListProps = PropsWithChildren & SxProp + +const NavListImpl = forwardRef(function NavList(props, ref) { + return +}) + +type NavListItemProps = PropsWithChildren & SxProp + +const NavListItem = forwardRef(function NavListItem(props, ref) { + // @ts-expect-error - PrimerNavList.Item is not recognized as a valid component type + return +}) + +type NavListGroupProps = PropsWithChildren & SxProp + +const NavListGroup = forwardRef(function NavListGroup(props, ref) { + // @ts-expect-error - PrimerNavList.Group is not recognized as a valid component type + return +}) + +const NavList = Object.assign(NavListImpl, { + // Wrapped components that need sx support added back in + Item: NavListItem, + Group: NavListGroup, + + // Re-exporting others directly + // TODO: try to remove typecasts to work around "non-portable types" TS error + SubNav: PrimerNavList.SubNav as React.FC>>, + Divider: PrimerNavList.Divider as React.FC>>, + LeadingVisual: PrimerNavList.LeadingVisual as React.FC< + React.PropsWithChildren> + >, + TrailingVisual: PrimerNavList.TrailingVisual as React.FC< + React.PropsWithChildren> + >, + TrailingAction: PrimerNavList.TrailingAction as React.FC< + React.PropsWithChildren> + >, + GroupHeading: PrimerNavList.GroupHeading as React.FC< + React.PropsWithChildren> + >, + GroupExpand: PrimerNavList.GroupExpand as React.FC< + React.PropsWithChildren> + >, +}) + +export {NavList, type NavListProps} diff --git a/packages/styled-react/src/index.tsx b/packages/styled-react/src/index.tsx index c21bf22b9b0..2dbcfe783eb 100644 --- a/packages/styled-react/src/index.tsx +++ b/packages/styled-react/src/index.tsx @@ -12,7 +12,6 @@ export {FormControl} from '@primer/react' export {Heading} from '@primer/react' export {IconButton} from '@primer/react' export {Label} from '@primer/react' -export {NavList} from '@primer/react' export {Overlay} from '@primer/react' export {ProgressBar} from '@primer/react' export {Select} from '@primer/react' @@ -39,6 +38,7 @@ export {Flash} from './components/Flash' export {Header, type HeaderProps} from './components/Header' export {Link, type LinkProps} from './components/Link' export {LinkButton, type LinkButtonProps} from './components/LinkButton' +export {NavList, type NavListProps} from './components/NavList' export {PageLayout, type PageLayoutProps} from './components/PageLayout' export { PageHeader,