From 0e0f03fcde910717418c58b3d8df41b1386634e9 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Fri, 5 Apr 2024 17:40:21 +0200 Subject: [PATCH] Revert "Add support for nested submenus to `ActionMenu` (#4386)" This reverts commit 4e281b2e65de237969f5a48c93dd4e4e3c18b72f. --- .changeset/wild-students-bow.md | 5 - .../ActionList/ActionListContainerContext.tsx | 1 - packages/react/src/ActionList/Item.tsx | 14 +- .../ActionMenu.features.stories.tsx | 53 +----- packages/react/src/ActionMenu/ActionMenu.tsx | 96 ++-------- .../react/src/__tests__/ActionMenu.test.tsx | 169 +----------------- .../src/hooks/useMenuKeyboardNavigation.ts | 31 +--- 7 files changed, 22 insertions(+), 347 deletions(-) delete mode 100644 .changeset/wild-students-bow.md diff --git a/.changeset/wild-students-bow.md b/.changeset/wild-students-bow.md deleted file mode 100644 index 111e2d290e9..00000000000 --- a/.changeset/wild-students-bow.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@primer/react": minor ---- - -Adds support for nested submenus to `ActionMenu` diff --git a/packages/react/src/ActionList/ActionListContainerContext.tsx b/packages/react/src/ActionList/ActionListContainerContext.tsx index 57370225370..1127042aa56 100644 --- a/packages/react/src/ActionList/ActionListContainerContext.tsx +++ b/packages/react/src/ActionList/ActionListContainerContext.tsx @@ -14,7 +14,6 @@ type ContextProps = { // eslint-disable-next-line @typescript-eslint/ban-types afterSelect?: Function enableFocusZone?: boolean - defaultTrailingVisual?: React.ReactElement } export const ActionListContainerContext = React.createContext({}) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 640f4885541..e7e14055d3e 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -74,15 +74,6 @@ export const Item = React.forwardRef( inlineDescription: [Description, props => props.variant !== 'block'], }) - 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 { variant: listVariant, role: listRole, @@ -90,6 +81,7 @@ export const Item = React.forwardRef( selectionVariant: listSelectionVariant, } = React.useContext(ListContext) const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext) + const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext) const inactive = Boolean(inactiveText) const showInactiveIndicator = inactive && container === undefined @@ -316,7 +308,7 @@ export const Item = React.forwardRef( sx={{display: 'flex', flexDirection: 'column', flexGrow: 1, minWidth: 0}} > ( ) : ( // If it's not inactive, or it has a leading visual that can be replaced, // just render the trailing visual slot. - trailingVisual + slots.trailingVisual ) } diff --git a/packages/react/src/ActionMenu/ActionMenu.features.stories.tsx b/packages/react/src/ActionMenu/ActionMenu.features.stories.tsx index 670e3b49cc8..f417e8cb1f4 100644 --- a/packages/react/src/ActionMenu/ActionMenu.features.stories.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.features.stories.tsx @@ -1,15 +1,6 @@ import React from 'react' import {ActionMenu, ActionList, Box} from '../' -import { - WorkflowIcon, - ArchiveIcon, - GearIcon, - CopyIcon, - RocketIcon, - CommentIcon, - BookIcon, - SparkleFillIcon, -} from '@primer/octicons-react' +import {WorkflowIcon, ArchiveIcon, GearIcon, CopyIcon, RocketIcon, CommentIcon, BookIcon} from '@primer/octicons-react' export default { title: 'Components/ActionMenu/Features', @@ -190,45 +181,3 @@ export const InactiveItems = () => ( ) - -export const Submenus = () => ( - - Edit - - - Cut - Copy - Paste - - - - - - - Paste special - - - - - Paste plain text - Paste formulas - Paste with formatting - - - Paste from - - - - Current clipboard - History - Another device - - - - - - - - - -) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 5ef5008e711..b549e1b23e0 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -1,5 +1,5 @@ -import React, {useCallback, useContext, useMemo} from 'react' -import {TriangleDownIcon, ChevronRightIcon} from '@primer/octicons-react' +import React from 'react' +import {TriangleDownIcon} from '@primer/octicons-react' import type {AnchoredOverlayProps} from '../AnchoredOverlay' import {AnchoredOverlay} from '../AnchoredOverlay' import type {OverlayProps} from '../Overlay' @@ -13,16 +13,11 @@ import type {MandateProps} from '../utils/types' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {Tooltip} from '../TooltipV2/Tooltip' -export type MenuCloseHandler = ( - gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab' | 'item-select' | 'arrow-left', -) => void - export type MenuContextProps = Pick< AnchoredOverlayProps, 'anchorRef' | 'renderAnchor' | 'open' | 'onOpen' | 'anchorId' > & { - onClose?: MenuCloseHandler - isSubmenu?: boolean + onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void } const MenuContext = React.createContext({renderAnchor: null, open: false}) @@ -49,23 +44,9 @@ const Menu: React.FC> = ({ onOpenChange, children, }: ActionMenuProps) => { - const parentMenuContext = useContext(MenuContext) - const [combinedOpenState, setCombinedOpenState] = useProvidedStateOrCreate(open, onOpenChange, false) const onOpen = React.useCallback(() => setCombinedOpenState(true), [setCombinedOpenState]) - const onClose: MenuCloseHandler = React.useCallback( - gesture => { - setCombinedOpenState(false) - - // Close the parent stack when an item is selected or the user tabs out of the menu entirely - switch (gesture) { - case 'tab': - case 'item-select': - parentMenuContext.onClose?.(gesture) - } - }, - [setCombinedOpenState, parentMenuContext], - ) + const onClose = React.useCallback(() => setCombinedOpenState(false), [setCombinedOpenState]) const menuButtonChild = React.Children.toArray(children).find( child => React.isValidElement(child) && (child.type === MenuButton || child.type === Anchor), @@ -119,18 +100,7 @@ const Menu: React.FC> = ({ }) return ( - + {contents} ) @@ -138,40 +108,7 @@ const Menu: React.FC> = ({ export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} const Anchor = React.forwardRef(({children, ...anchorProps}, anchorRef) => { - const {onOpen, isSubmenu} = React.useContext(MenuContext) - - const openSubmenuOnRightArrow: React.KeyboardEventHandler = useCallback( - event => { - children.props.onKeyDown?.(event) - if (isSubmenu && event.key === 'ArrowRight' && !event.defaultPrevented) onOpen?.('anchor-key-press') - }, - [children, isSubmenu, onOpen], - ) - - // Add right chevron icon to submenu anchors rendered using `ActionList.Item` - const parentActionListContext = useContext(ActionListContainerContext) - const thisActionListContext = useMemo( - () => - isSubmenu - ? { - ...parentActionListContext, - defaultTrailingVisual: , - // Default behavior is to close after selecting; we want to open the submenu instead - afterSelect: () => onOpen?.('anchor-click'), - } - : parentActionListContext, - [isSubmenu, onOpen, parentActionListContext], - ) - - return ( - - {React.cloneElement(children, { - ...anchorProps, - ref: anchorRef, - onKeyDown: openSubmenuOnRightArrow, - })} - - ) + return React.cloneElement(children, {...anchorProps, ref: anchorRef}) }) /** this component is syntactical sugar 🍭 */ @@ -196,24 +133,19 @@ type MenuOverlayProps = Partial & const Overlay: React.FC> = ({ children, align = 'start', - side, + side = 'outside-bottom', '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, - isSubmenu = false, - } = React.useContext(MenuContext) as MandateProps + const {anchorRef, renderAnchor, anchorId, open, onOpen, onClose} = React.useContext(MenuContext) as MandateProps< + MenuContextProps, + 'anchorRef' + > const containerRef = React.useRef(null) - useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef, isSubmenu) + useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef) return ( > = ({ onOpen={onOpen} onClose={onClose} align={align} - side={side ?? (isSubmenu ? 'outside-right' : 'outside-bottom')} + side={side} overlayProps={overlayProps} focusZoneSettings={{focusOutBehavior: 'wrap'}} > @@ -235,7 +167,7 @@ const Overlay: React.FC> = ({ listRole: 'menu', listLabelledBy: ariaLabelledby || anchorId, selectionAttribute: 'aria-checked', // Should this be here? - afterSelect: () => onClose?.('item-select'), + afterSelect: onClose, }} > {children} diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index 5598e686de2..cb5f52e915d 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -1,4 +1,4 @@ -import {render as HTMLRender, waitFor, act, within} from '@testing-library/react' +import {render as HTMLRender, waitFor, act} from '@testing-library/react' import userEvent from '@testing-library/user-event' import {axe} from 'jest-axe' import React from 'react' @@ -78,60 +78,6 @@ function ExampleWithTooltipV2(actionMenuTrigger: React.ReactElement): JSX.Elemen ) } -function ExampleWithSubmenus(): JSX.Element { - return ( - - - - - Toggle Menu - - - New file - - Copy link - Edit file - - Paste - - - Paste special - - - - Paste plain text - Paste formulas - Paste with formatting - - - Paste from - - - - { - /*noop*/ - }} - > - Current clipboard - - History - Another device - - - - - - - - - - - - - ) -} - describe('ActionMenu', () => { behavesAsComponent({ Component: ActionList, @@ -486,117 +432,4 @@ describe('ActionMenu', () => { expect(button.id).toBe(buttonId) }) - - describe('submenus', () => { - it('sets `aria-haspopup` and `aria-expanded` on submenu anchors', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - await user.click(baseAnchor) - - const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) - expect(submenuAnchor).toHaveAttribute('aria-haspopup') - await user.click(submenuAnchor) - expect(submenuAnchor).toHaveAttribute('aria-expanded') - - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) - expect(subSubmenuAnchor).toHaveAttribute('aria-haspopup') - await user.click(subSubmenuAnchor) - expect(subSubmenuAnchor).toHaveAttribute('aria-expanded') - }) - - it('sets labels on submenus', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - await user.click(baseAnchor) - - const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) - await user.click(submenuAnchor) - const submenu = component.getByRole('menu', {name: 'Paste special'}) - expect(submenu).toBeVisible() - - const subSubmenuAnchor = within(submenu).getByRole('menuitem', {name: 'Paste from'}) - await user.click(subSubmenuAnchor) - const subSubmenu = component.getByRole('menu', {name: 'Paste from'}) - expect(subSubmenu).toBeVisible() - }) - - it('does not open top-level menu on right arrow key press', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - baseAnchor.focus() - - await user.keyboard('{ArrowRight}') - expect(component.queryByRole('menu')).not.toBeInTheDocument() - expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true') - }) - - it('opens submenus on enter or right arrow key press', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - await user.click(baseAnchor) - - const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) - expect(submenuAnchor).toHaveAttribute('aria-haspopup', 'true') - submenuAnchor.focus() - await user.keyboard('{Enter}') - expect(submenuAnchor).toHaveAttribute('aria-expanded', 'true') - - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) - subSubmenuAnchor.focus() - await user.keyboard('{ArrowRight}') - expect(subSubmenuAnchor).toHaveAttribute('aria-expanded', 'true') - }) - - it('closes top menu on escape or left arrow key press', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - await user.click(baseAnchor) - - const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) - await user.click(submenuAnchor) - - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) - await user.click(subSubmenuAnchor) - - expect(subSubmenuAnchor).toHaveAttribute('aria-expanded', 'true') - - await user.keyboard('{Escape}') - expect(subSubmenuAnchor).not.toHaveAttribute('aria-expanded', 'true') - expect(submenuAnchor).toHaveAttribute('aria-expanded', 'true') - - await user.keyboard('{ArrowLeft}') - expect(submenuAnchor).not.toHaveAttribute('aria-expanded', 'true') - - expect(baseAnchor).toHaveAttribute('aria-expanded', 'true') - }) - - it('closes all menus when an item is selected', async () => { - const component = HTMLRender() - const user = userEvent.setup() - - const baseAnchor = component.getByRole('button', {name: 'Toggle Menu'}) - await user.click(baseAnchor) - - const submenuAnchor = component.getByRole('menuitem', {name: 'Paste special'}) - await user.click(submenuAnchor) - - const subSubmenuAnchor = component.getByRole('menuitem', {name: 'Paste from'}) - await user.click(subSubmenuAnchor) - - const subSubmenuItem = component.getByRole('menuitem', {name: 'Current clipboard'}) - await user.click(subSubmenuItem) - - expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true') - }) - }) }) diff --git a/packages/react/src/hooks/useMenuKeyboardNavigation.ts b/packages/react/src/hooks/useMenuKeyboardNavigation.ts index a11fb7d8fb0..8adbb52778a 100644 --- a/packages/react/src/hooks/useMenuKeyboardNavigation.ts +++ b/packages/react/src/hooks/useMenuKeyboardNavigation.ts @@ -2,7 +2,7 @@ import React from 'react' import {iterateFocusableElements} from '@primer/behaviors/utils' import {useMenuInitialFocus} from './useMenuInitialFocus' import {useMnemonics} from './useMnemonics' -import type {MenuCloseHandler} from '../ActionMenu' +import type {MenuContextProps} from '../ActionMenu' /** * Keyboard navigation is a mix of 4 hooks @@ -13,16 +13,14 @@ import type {MenuCloseHandler} from '../ActionMenu' */ export const useMenuKeyboardNavigation = ( open: boolean, - onClose: MenuCloseHandler | undefined, + onClose: MenuContextProps['onClose'], containerRef: React.RefObject, anchorRef: React.RefObject, - isSubmenu: boolean, ) => { useMenuInitialFocus(open, containerRef, anchorRef) useMnemonics(open, containerRef) useCloseMenuOnTab(open, onClose, containerRef, anchorRef) useMoveFocusToMenuItem(open, containerRef, anchorRef) - useCloseSubmenuOnArrow(open, isSubmenu, onClose, containerRef) } /** @@ -31,7 +29,7 @@ export const useMenuKeyboardNavigation = ( */ const useCloseMenuOnTab = ( open: boolean, - onClose: MenuCloseHandler | undefined, + onClose: MenuContextProps['onClose'], containerRef: React.RefObject, anchorRef: React.RefObject, ) => { @@ -52,29 +50,6 @@ const useCloseMenuOnTab = ( }, [open, onClose, containerRef, anchorRef]) } -/** - * Close submenu when left arrow key is pressed. - */ -const useCloseSubmenuOnArrow = ( - open: boolean, - isSubmenu: boolean, - onClose: MenuCloseHandler | undefined, - containerRef: React.RefObject, -) => { - React.useEffect(() => { - const container = containerRef.current - - const handler = (event: KeyboardEvent) => { - if (open && isSubmenu && event.key === 'ArrowLeft') onClose?.('arrow-left') - } - - container?.addEventListener('keydown', handler) - return () => { - container?.removeEventListener('keydown', handler) - } - }, [open, onClose, containerRef, isSubmenu]) -} - /** * When Arrow Keys are pressed and the focus is on the anchor, * focus should move to a menu item