Skip to content

Commit

Permalink
Revert "Refactor(ActionList): ActionList.Item should render content a…
Browse files Browse the repository at this point in the history
…s a button if parent is not interactive. (#3284)"

This reverts commit 08d7d5d.
  • Loading branch information
broccolinisoup authored Jun 12, 2023
1 parent 58b118f commit 53ec7b1
Show file tree
Hide file tree
Showing 14 changed files with 280 additions and 297 deletions.
5 changes: 0 additions & 5 deletions .changeset/purple-crabs-matter.md

This file was deleted.

1 change: 1 addition & 0 deletions src/ActionList/Description.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const Description: React.FC<React.PropsWithChildren<ActionListDescription
sx={merge({...styles, color: disabled ? 'fg.disabled' : 'fg.muted'}, sx as SxProp)}
title={props.children as string}
inline={true}
maxWidth="100%"
>
{props.children}
</Truncate>
Expand Down
62 changes: 11 additions & 51 deletions src/ActionList/Item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ import {defaultSxProp} from '../utils/defaultSxProp'
import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
import {ActionListContainerContext} from './ActionListContainerContext'
import {Description} from './Description'
import {ListContext} from './List'
import {ActionListProps, ListContext} from './List'
import {Selection} from './Selection'
import {ActionListItemProps, getVariantStyles, ItemContext, TEXT_ROW_HEIGHT} from './shared'
import {LeadingVisual, TrailingVisual} from './Visuals'
import {MenuContext} from '../ActionMenu/ActionMenu'
import {GroupContext} from './Group'

const LiBox = styled.li<SxProp>(sx)
Expand All @@ -30,8 +29,6 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
id,
role,
_PrivateItemWrapper,
// @ts-ignore tabIndex is sometimes passed as a prop in dotcom.
tabIndex,
...props
},
forwardedRef,
Expand All @@ -41,13 +38,10 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
trailingVisual: TrailingVisual,
description: Description,
})

const {variant: listVariant, showDividers, selectionVariant: listSelectionVariant} = React.useContext(ListContext)
const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext)
const menuContext = React.useContext(MenuContext)
const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)

const selectionVariant = groupSelectionVariant ?? listSelectionVariant
const onSelect = React.useCallback(
(
event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>,
Expand All @@ -61,6 +55,10 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
[onSelectUser],
)

const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant
? groupSelectionVariant
: listSelectionVariant

/** Infer item role based on the container */
let itemRole: ActionListItemProps['role']
if (container === 'ActionMenu' || container === 'DropdownMenu') {
Expand All @@ -86,22 +84,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
},
}

const isTopLevelInteractive = () =>
_PrivateItemWrapper !== undefined ||
// @ts-ignore props.as may be defined, may not.
props.as === 'button' ||
// @ts-ignore props.as may be defined, may not.
props.as === 'a' ||
menuContext.anchorId !== undefined ||
role?.match(/menuitem/) ||
tabIndex !== undefined

const styles = {
position: 'relative',
display: 'flex',
paddingX: isTopLevelInteractive() ? 2 : 0,
paddingX: 2,
fontSize: 1,
paddingY: isTopLevelInteractive() ? '6px' : 0, // custom value off the scale
paddingY: '6px', // custom value off the scale
lineHeight: TEXT_ROW_HEIGHT,
minHeight: 5,
marginX: listVariant === 'inset' ? 2 : 0,
Expand Down Expand Up @@ -157,10 +145,6 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
borderTopWidth: showDividers ? `1px` : '0',
borderColor: 'var(--divider-color, transparent)',
},
'button[data-component="ActionList.Item--DividerContainer"]': {
textAlign: 'left',
padding: 0,
},
// show between 2 items
':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider},
// hide divider after dividers & group header, with higher importance!
Expand Down Expand Up @@ -198,13 +182,13 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
const inlineDescriptionId = useId(id && `${id}--inline-description`)
const blockDescriptionId = useId(id && `${id}--block-description`)

const ItemWrapper = _PrivateItemWrapper || Box
const ItemWrapper = _PrivateItemWrapper || React.Fragment

const menuItemProps = {
onClick: clickHandler,
onKeyPress: keyPressHandler,
'aria-disabled': disabled ? true : undefined,
tabIndex: disabled || !isTopLevelInteractive() ? undefined : 0,
tabIndex: disabled ? undefined : 0,
'aria-labelledby': `${labelId} ${
slots.description && slots.description.props.variant !== 'block' ? inlineDescriptionId : ''
}`,
Expand All @@ -215,41 +199,17 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(

const containerProps = _PrivateItemWrapper ? {role: role || itemRole ? 'none' : undefined} : menuItemProps

const wrapperProps = _PrivateItemWrapper
? menuItemProps
: {
sx: {
display: 'flex',
paddingX: isTopLevelInteractive() ? 0 : 2,
paddingY: isTopLevelInteractive() ? 0 : '6px', // custom value off the scale
flexGrow: 1,
},
}
const wrapperProps = _PrivateItemWrapper ? menuItemProps : {}

return (
<ItemContext.Provider value={{variant, disabled, inlineDescriptionId, blockDescriptionId}}>
<LiBox ref={forwardedRef} sx={merge<BetterSystemStyleObject>(styles, sxProp)} {...containerProps} {...props}>
{/* @ts-ignore onClick prop is only passed when _PrivateItemWrapper is set by ActionList.LinkItem. */}
<ItemWrapper {...wrapperProps}>
<Selection selected={selected} />
{slots.leadingVisual}
<Box
data-component="ActionList.Item--DividerContainer"
sx={{
display: 'flex',
flexDirection: 'column',
flexGrow: 1,
minWidth: 0,
borderStyle: 'none',
backgroundColor: 'transparent',
cursor: 'inherit',
fontSize: 'inherit',
color: getVariantStyles(variant, disabled).color,
lineHeight: '20px',
}}
// @ts-ignore `as` prop may be passed to ActionList.Item, even if it isn't defined in ActionListItemProps.
// If this item is inside an ActionMenu, don't render an interactive button.
as={isTopLevelInteractive() ? 'div' : 'button'}
sx={{display: 'flex', flexDirection: 'column', flexGrow: 1, minWidth: 0}}
>
<ConditionalBox if={Boolean(slots.trailingVisual)} sx={{display: 'flex', flexGrow: 1}}>
<ConditionalBox
Expand Down
138 changes: 138 additions & 0 deletions src/ActionMenu.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
import React from 'react'
import {TriangleDownIcon} from '@primer/octicons-react'
import {AnchoredOverlay, AnchoredOverlayProps} from './AnchoredOverlay'
import {OverlayProps} from './Overlay'
import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from './hooks'
import {Divider} from './ActionList/Divider'
import {ActionListContainerContext} from './ActionList/ActionListContainerContext'
import {Button, ButtonProps} from './Button'
import {useId} from './hooks/useId'
import {MandateProps} from './utils/types'
import {ForwardRefComponent as PolymorphicForwardRefComponent} from './utils/polymorphic'

export type MenuContextProps = Pick<
AnchoredOverlayProps,
'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'anchorId'
> & {
onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void
}
const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})

export type ActionMenuProps = {
/**
* Recommended: `ActionMenu.Button` or `ActionMenu.Anchor` with `ActionMenu.Overlay`
*/
children: React.ReactElement[] | React.ReactElement

/**
* If defined, will control the open/closed state of the overlay. Must be used in conjunction with `onOpenChange`.
*/
open?: boolean

/**
* If defined, will control the open/closed state of the overlay. Must be used in conjunction with `open`.
*/
onOpenChange?: (s: boolean) => void
} & Pick<AnchoredOverlayProps, 'anchorRef'>

const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
anchorRef: externalAnchorRef,
open,
onOpenChange,
children,
}: ActionMenuProps) => {
const [combinedOpenState, setCombinedOpenState] = useProvidedStateOrCreate(open, onOpenChange, false)
const onOpen = React.useCallback(() => setCombinedOpenState(true), [setCombinedOpenState])
const onClose = React.useCallback(() => setCombinedOpenState(false), [setCombinedOpenState])

const anchorRef = useProvidedRefOrCreate(externalAnchorRef)
const anchorId = useId()
let renderAnchor: AnchoredOverlayProps['renderAnchor'] = null

// 🚨 Hack for good API!
// we strip out Anchor from children and pass it to AnchoredOverlay to render
// with additional props for accessibility
const contents = React.Children.map(children, child => {
if (child.type === MenuButton || child.type === Anchor) {
renderAnchor = anchorProps => React.cloneElement(child, anchorProps)
return null
}
return child
})

return (
<MenuContext.Provider value={{anchorRef, renderAnchor, anchorId, open: combinedOpenState, onOpen, onClose}}>
{contents}
</MenuContext.Provider>
)
}

export type ActionMenuAnchorProps = {children: React.ReactElement}
const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children, ...anchorProps}, anchorRef) => {
return React.cloneElement(children, {...anchorProps, ref: anchorRef})
})

/** this component is syntactical sugar 🍭 */
export type ActionMenuButtonProps = ButtonProps
const MenuButton = React.forwardRef(({...props}, anchorRef) => {
return (
<Anchor ref={anchorRef}>
<Button type="button" trailingAction={TriangleDownIcon} {...props} />
</Anchor>
)
}) as PolymorphicForwardRefComponent<'button', ActionMenuButtonProps>

type MenuOverlayProps = Partial<OverlayProps> &
Pick<AnchoredOverlayProps, 'align'> & {
/**
* Recommended: `ActionList`
*/
children: React.ReactNode
}
const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
children,
align = 'start',
'aria-labelledby': ariaLabelledby,
...overlayProps
}) => {
// we typecast anchorRef as required instead of optional
// because we know that we're setting it in context in Menu
const {anchorRef, renderAnchor, anchorId, open, onOpen, onClose} = React.useContext(MenuContext) as MandateProps<
MenuContextProps,
'anchorRef'
>

const containerRef = React.useRef<HTMLDivElement>(null)
useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef)

return (
<AnchoredOverlay
anchorRef={anchorRef}
renderAnchor={renderAnchor}
anchorId={anchorId}
open={open}
onOpen={onOpen}
onClose={onClose}
align={align}
overlayProps={overlayProps}
focusZoneSettings={{focusOutBehavior: 'wrap'}}
>
<div ref={containerRef}>
<ActionListContainerContext.Provider
value={{
container: 'ActionMenu',
listRole: 'menu',
listLabelledBy: ariaLabelledby || anchorId,
selectionAttribute: 'aria-checked', // Should this be here?
afterSelect: onClose,
}}
>
{children}
</ActionListContainerContext.Provider>
</div>
</AnchoredOverlay>
)
}

Menu.displayName = 'ActionMenu'
export const ActionMenu = Object.assign(Menu, {Button: MenuButton, Anchor, Overlay, Divider})
3 changes: 1 addition & 2 deletions src/ActionMenu/ActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ export type MenuContextProps = Pick<
> & {
onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void
}

export const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})
const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})

export type ActionMenuProps = {
/**
Expand Down
2 changes: 1 addition & 1 deletion src/NavList/NavList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ describe('NavList.Item with NavList.SubNav', () => {
</ThemeProvider>,
)

const button = getByRole('button', {name: 'Item'})
const button = getByRole('button')

// Starts open
expect(queryByRole('list', {name: 'Item'})).toBeVisible()
Expand Down
Loading

0 comments on commit 53ec7b1

Please sign in to comment.