From c49d855d92d2d372bad9daba144a4f0dd3776179 Mon Sep 17 00:00:00 2001 From: Jeremy Neal Date: Tue, 6 Jun 2023 13:49:14 -0400 Subject: [PATCH] Refactor(ActionList): ActionList.Item should render content as a button if parent is not interactive. (#3284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Using button as the underlying tag for ActionList.Item content. * Fix subnav layout. * Changing ActionList content to button if the top-level is not a button. ActionMenu is busted. * Fix issue where ActionList.Item content would render as a button inside ActionMenu, breaking keyboard navigation. * Create purple-crabs-matter.md * test(vrt): update snapshots * Fix bug with MenuContext not being exported properly. * Fix color violation in ActionList.Item. * Formatting, update snapshots. * test(vrt): update snapshots * Fix a11y violation in ActionList story. * Formatting. * Fix ts-ignore comment. * Adjust line-height when button is rendered. * Revert "test(vrt): update snapshots" This reverts commit ba6d863a69341e0792f44f0185dc336e298775dc. * Revert "test(vrt): update snapshots" This reverts commit 31d5358ad85ef83803434aaa09ea016602337e0d. * Update snapshots. * Remove flexGrow from ActionList.Item to remove extra 1px vertical height. * Set padding to 0 and put flexGrow back to fix padding issue for ActionList buttons. * Fix ActionList text wrap story. * Fix underlinenav story by checking ActionList.Item if as prop is a. * Update snapshots and formatting. * Update snapshots. * If ActionList.Item content is rendered as a button, remove tabIndex from li and fix padding. * Fix various layout edge cases. * ts-ignore event handlers on ItemWrapper. * Update snapshots. * Revert fontWeight config that differed from production docs. * Pass selectionVariant prop illegally in UnderlineNav story to fix issue with Selection spans showing up. * fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler by @gr2m (#3346) * fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler (#3163) * test: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler Failing test for https://github.com/primer/react/issues/3162 * fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`ߴs `onSelect` handler * add storybook example: Delayed Menu Close * update docs * docs: changeset * Update changelog --------- Co-authored-by: Siddharth Kshetrapal * Update generated/components.json --------- Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com> Co-authored-by: siddharthkp * Check for tabIndex value in isTopLevelInteractive. * Reference styles in updated menuProps. * Updated snapshots. * Fix padding setting instead of using values from styles, which are inverted. * Update snapshots. --------- Co-authored-by: radglob Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com> Co-authored-by: siddharthkp --- .changeset/purple-crabs-matter.md | 5 + src/ActionList/Description.tsx | 1 - src/ActionList/Item.tsx | 62 ++- src/ActionMenu.tsx | 138 ------- .../ActionMenu.test.tsx | 3 +- src/ActionMenu/ActionMenu.tsx | 3 +- .../ActionMenu.types.test.tsx | 0 .../__snapshots__/ActionMenu.test.tsx.snap | 0 src/NavList/NavList.test.tsx | 2 +- .../__snapshots__/NavList.test.tsx.snap | 352 +++++++++++------- src/UnderlineNav2/UnderlineNav.tsx | 2 + .../UnderlineNav2.features.stories.tsx | 6 +- .../__snapshots__/exports.test.ts.snap | 1 + src/index.ts | 2 +- 14 files changed, 297 insertions(+), 280 deletions(-) create mode 100644 .changeset/purple-crabs-matter.md delete mode 100644 src/ActionMenu.tsx rename src/{__tests__ => ActionMenu}/ActionMenu.test.tsx (98%) rename src/{__tests__ => ActionMenu}/ActionMenu.types.test.tsx (100%) rename src/{__tests__ => ActionMenu}/__snapshots__/ActionMenu.test.tsx.snap (100%) diff --git a/.changeset/purple-crabs-matter.md b/.changeset/purple-crabs-matter.md new file mode 100644 index 00000000000..261665de8d8 --- /dev/null +++ b/.changeset/purple-crabs-matter.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Refactor(ActionList): ActionList.Item should render content as a button if parent is not interactive. diff --git a/src/ActionList/Description.tsx b/src/ActionList/Description.tsx index 4027ec22f58..011c1a800b8 100644 --- a/src/ActionList/Description.tsx +++ b/src/ActionList/Description.tsx @@ -44,7 +44,6 @@ export const Description: React.FC {props.children} diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 56e91d0070c..c0b3eae74b8 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -9,10 +9,11 @@ import {defaultSxProp} from '../utils/defaultSxProp' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {ActionListContainerContext} from './ActionListContainerContext' import {Description} from './Description' -import {ActionListProps, ListContext} from './List' +import {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(sx) @@ -29,6 +30,8 @@ export const Item = React.forwardRef( id, role, _PrivateItemWrapper, + // @ts-ignore tabIndex is sometimes passed as a prop in dotcom. + tabIndex, ...props }, forwardedRef, @@ -38,10 +41,13 @@ export const Item = React.forwardRef( 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 | React.KeyboardEvent, @@ -55,10 +61,6 @@ export const Item = React.forwardRef( [onSelectUser], ) - const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant - ? groupSelectionVariant - : listSelectionVariant - /** Infer item role based on the container */ let itemRole: ActionListItemProps['role'] if (container === 'ActionMenu' || container === 'DropdownMenu') { @@ -84,12 +86,22 @@ export const Item = React.forwardRef( }, } + 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: 2, + paddingX: isTopLevelInteractive() ? 2 : 0, fontSize: 1, - paddingY: '6px', // custom value off the scale + paddingY: isTopLevelInteractive() ? '6px' : 0, // custom value off the scale lineHeight: TEXT_ROW_HEIGHT, minHeight: 5, marginX: listVariant === 'inset' ? 2 : 0, @@ -145,6 +157,10 @@ export const Item = React.forwardRef( 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! @@ -182,13 +198,13 @@ export const Item = React.forwardRef( const inlineDescriptionId = useId(id && `${id}--inline-description`) const blockDescriptionId = useId(id && `${id}--block-description`) - const ItemWrapper = _PrivateItemWrapper || React.Fragment + const ItemWrapper = _PrivateItemWrapper || Box const menuItemProps = { onClick: clickHandler, onKeyPress: keyPressHandler, 'aria-disabled': disabled ? true : undefined, - tabIndex: disabled ? undefined : 0, + tabIndex: disabled || !isTopLevelInteractive() ? undefined : 0, 'aria-labelledby': `${labelId} ${ slots.description && slots.description.props.variant !== 'block' ? inlineDescriptionId : '' }`, @@ -199,17 +215,41 @@ export const Item = React.forwardRef( const containerProps = _PrivateItemWrapper ? {role: role || itemRole ? 'none' : undefined} : menuItemProps - const wrapperProps = _PrivateItemWrapper ? menuItemProps : {} + const wrapperProps = _PrivateItemWrapper + ? menuItemProps + : { + sx: { + display: 'flex', + paddingX: isTopLevelInteractive() ? 0 : 2, + paddingY: isTopLevelInteractive() ? 0 : '6px', // custom value off the scale + flexGrow: 1, + }, + } return ( (styles, sxProp)} {...containerProps} {...props}> + {/* @ts-ignore onClick prop is only passed when _PrivateItemWrapper is set by ActionList.LinkItem. */} {slots.leadingVisual} & { - onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void -} -const MenuContext = React.createContext({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 - -const Menu: React.FC> = ({ - 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 ( - - {contents} - - ) -} - -export type ActionMenuAnchorProps = {children: React.ReactElement} -const Anchor = React.forwardRef(({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 ( - -