From e65e7f3544e7711831ad4087482720503a9078fc Mon Sep 17 00:00:00 2001 From: Joss Mackison <2730833+jossmac@users.noreply.github.com> Date: Wed, 4 Dec 2024 20:10:33 +1100 Subject: [PATCH] Menu hold affordance (#1370) --- .changeset/smart-monkeys-leave.md | 5 + design-system/pkg/src/menu/MenuTrigger.tsx | 26 +- .../pkg/src/menu/stories/Menu.stories.tsx | 7 - .../pkg/src/menu/test/MenuTrigger.test.tsx | 222 ------------------ design-system/pkg/src/menu/types.ts | 4 +- 5 files changed, 17 insertions(+), 247 deletions(-) create mode 100644 .changeset/smart-monkeys-leave.md diff --git a/.changeset/smart-monkeys-leave.md b/.changeset/smart-monkeys-leave.md new file mode 100644 index 000000000..da1cfe054 --- /dev/null +++ b/.changeset/smart-monkeys-leave.md @@ -0,0 +1,5 @@ +--- +'@keystar/ui': patch +--- + +Remove unused "longpress" behaviour on `Menu` component. diff --git a/design-system/pkg/src/menu/MenuTrigger.tsx b/design-system/pkg/src/menu/MenuTrigger.tsx index 720c4a835..e573b247c 100644 --- a/design-system/pkg/src/menu/MenuTrigger.tsx +++ b/design-system/pkg/src/menu/MenuTrigger.tsx @@ -6,7 +6,6 @@ import { Placement } from '@react-types/overlays'; import React, { ForwardedRef, forwardRef, Fragment, useRef } from 'react'; import { Popover, Tray } from '@keystar/ui/overlays'; -import { SlotProvider } from '@keystar/ui/slots'; import { tokenSchema, useIsMobileDevice } from '@keystar/ui/style'; import { MenuContext } from './context'; @@ -25,19 +24,18 @@ export const MenuTrigger = forwardRef(function MenuTrigger( let menuTriggerRef = domRef || triggerRef; let menuRef = useRef(null); let { - children, align = 'start', - shouldFlip = true, - direction = 'bottom', + children, closeOnSelect, - trigger = 'press', + direction = 'bottom', + shouldFlip = true, } = props; let [menuTrigger, menu] = React.Children.toArray(children); let state = useMenuTriggerState(props); let { menuTriggerProps, menuProps } = useMenuTrigger( - { trigger }, + { trigger: 'press' }, state, menuTriggerRef ); @@ -96,17 +94,13 @@ export const MenuTrigger = forwardRef(function MenuTrigger( return ( - - - {menuTrigger} - - + {menuTrigger} + - render(defaultMenu, { trigger: 'longPress' }); - -MenuTriggerWithTriggerLongPress.story = { - name: 'MenuTrigger with trigger="longPress"', -}; - // .add('controlled isOpen', () => ) let customMenuItem = (item: any) => { diff --git a/design-system/pkg/src/menu/test/MenuTrigger.test.tsx b/design-system/pkg/src/menu/test/MenuTrigger.test.tsx index 5a2f6d0f9..92aeb7fb7 100644 --- a/design-system/pkg/src/menu/test/MenuTrigger.test.tsx +++ b/design-system/pkg/src/menu/test/MenuTrigger.test.tsx @@ -16,22 +16,16 @@ import { Section, } from '..'; import { Button, ButtonProps } from '@keystar/ui/button'; -import { SelectionMode } from '@react-types/shared'; import { createRef } from 'react'; import { RenderResult, act, - DEFAULT_LONG_PRESS_TIME, fireEvent, - fireLongPress, firePress, - fireTouch, - installPointerEvent, render, renderWithProvider, within, KEYS, - waitFor, } from '#test-utils'; let triggerText = 'Menu Button'; @@ -760,222 +754,6 @@ describe('menu/MenuTrigger', () => { let menu2Item1 = within(menu2).getByText('Whiskey'); expect(menu2Item1).toBeInTheDocument(); }); - - describe('trigger="longPress" open behavior', function () { - installPointerEvent(); - - const ERROR_MENU_NOT_FOUND = new Error('Menu not found'); - const getMenuOrThrow = (tree: RenderResult, button: HTMLElement) => { - try { - let menu = tree.getByRole('menu'); - expect(menu).toBeTruthy(); - expect(menu).toHaveAttribute('aria-labelledby', button.id); - } catch (e) { - throw ERROR_MENU_NOT_FOUND; - } - }; - - it('should open the menu on longPress', function () { - verifyMenuToggle( - { onOpenChange, trigger: 'longPress' }, - {}, - (button, menu) => { - expect(button).toHaveAttribute('aria-describedby'); - expect( - document.getElementById(button.getAttribute('aria-describedby')!) - ).toHaveTextContent( - 'Long press or press Alt + ArrowDown to open menu' - ); - - if (!menu) { - fireLongPress(button); - } else { - fireTouch(button); - } - } - ); - }); - - it('should not open menu on click', function () { - let tree = renderComponent({ onOpenChange, trigger: 'longPress' }); - let button = tree.getByRole('button'); - - act(() => { - fireTouch(button); - setTimeout( - () => { - expect(getMenuOrThrow).toThrow(ERROR_MENU_NOT_FOUND); - }, - 0, - tree, - button - ); - jest.runAllTimers(); - }); - }); - - it(`should not open menu on short press (default threshold set to ${DEFAULT_LONG_PRESS_TIME}ms)`, function () { - let tree = renderComponent({ onOpenChange, trigger: 'longPress' }); - let button = tree.getByRole('button'); - - act(() => { - fireTouch(button); - setTimeout( - () => { - expect(getMenuOrThrow).toThrow(ERROR_MENU_NOT_FOUND); - }, - DEFAULT_LONG_PRESS_TIME / 2, - tree, - button - ); - jest.runAllTimers(); - }); - }); - - it('should not open the menu on Enter', function () { - let tree = renderComponent({ onOpenChange, trigger: 'longPress' }); - let button = tree.getByRole('button'); - act(() => { - fireTouch(button); - setTimeout( - () => { - expect(getMenuOrThrow).toThrow(ERROR_MENU_NOT_FOUND); - }, - 0, - tree, - button - ); - jest.runAllTimers(); - }); - }); - - it('should not open the menu on Space', function () { - let tree = renderComponent({ onOpenChange, trigger: 'longPress' }); - let button = tree.getByRole('button'); - act(() => { - fireTouch(button); - setTimeout( - () => { - expect(getMenuOrThrow).toThrow(ERROR_MENU_NOT_FOUND); - }, - 0, - tree, - button - ); - jest.runAllTimers(); - }); - }); - - it('should open the menu on Alt+ArrowUp', function () { - verifyMenuToggle( - { onOpenChange, trigger: 'longPress' }, - {}, - (button, menu) => { - if (!menu) { - fireEvent.keyDown(button, { key: 'ArrowUp', altKey: true }); - } else { - fireTouch(button); - } - } - ); - }); - - it('should open the menu on Alt+ArrowDown', function () { - verifyMenuToggle( - { onOpenChange, trigger: 'longPress' }, - {}, - (button, menu) => { - if (!menu) { - fireEvent.keyDown(button, { key: 'ArrowDown', altKey: true }); - } else { - fireTouch(button); - } - } - ); - }); - }); - - describe('trigger="longPress" focus behavior', function () { - installPointerEvent(); - - function expectMenuItemToBeActive( - tree: RenderResult, - idx: number, - selectionMode: SelectionMode - ) { - let menuItemRole = 'menuitem'; - if (selectionMode === 'multiple') { - menuItemRole = 'menuitemcheckbox'; - } else if (selectionMode === 'single') { - menuItemRole = 'menuitemradio'; - } - let menu = tree.getByRole('menu'); - expect(menu).toBeTruthy(); - let menuItems = within(menu).getAllByRole(menuItemRole); - let selectedItem = menuItems[idx < 0 ? menuItems.length + idx : idx]; - expect(selectedItem).toBe(document.activeElement); - return menu; - } - - it('should focus the selected item on menu open', async function () { - let tree = renderComponent( - { trigger: 'longPress' }, - { selectedKeys: ['Bar'], selectionMode: 'single' } - ); - let button = tree.getByRole('button'); - act(() => { - fireLongPress(button); - jest.runAllTimers(); - }); - let menu = expectMenuItemToBeActive(tree, 1, 'single'); - act(() => { - fireTouch(button); - jest.runAllTimers(); - }); - - await waitFor(() => { - expect(menu).not.toBeInTheDocument(); - }); - - // Opening menu via Alt+ArrowUp still autofocuses the selected item - fireEvent.keyDown(button, { key: 'ArrowUp', altKey: true }); - menu = expectMenuItemToBeActive(tree, 1, 'single'); - - act(() => { - fireTouch(button); - jest.runAllTimers(); - }); - await waitFor(() => { - expect(menu).not.toBeInTheDocument(); - }); - - // Opening menu via Alt+ArrowDown still autofocuses the selected item - fireEvent.keyDown(button, { key: 'ArrowDown', altKey: true }); - menu = expectMenuItemToBeActive(tree, 1, 'single'); - - act(() => { - fireTouch(button); - jest.runAllTimers(); - }); - await waitFor(() => { - expect(menu).not.toBeInTheDocument(); - }); - }); - - it('should focus the last item on Alt+ArrowUp if no selectedKeys specified', function () { - let tree = renderComponent({ trigger: 'longPress' }); - let button = tree.getByRole('button'); - fireEvent.keyDown(button, { key: 'ArrowUp', altKey: true }); - expectMenuItemToBeActive(tree, -1, 'none'); - }); - - it('should focus the first item on Alt+ArrowDown if no selectedKeys specified', function () { - let tree = renderComponent({ trigger: 'longPress' }); - let button = tree.getByRole('button'); - fireEvent.keyDown(button, { key: 'ArrowDown', altKey: true }); - expectMenuItemToBeActive(tree, 0, 'none'); - }); - }); }); function renderComponent( diff --git a/design-system/pkg/src/menu/types.ts b/design-system/pkg/src/menu/types.ts index c83cfc586..3562b9da7 100644 --- a/design-system/pkg/src/menu/types.ts +++ b/design-system/pkg/src/menu/types.ts @@ -1,6 +1,6 @@ import type { AriaMenuProps, - MenuTriggerProps as _MenuTriggerProps, + MenuTriggerProps as AriaMenuTriggerProps, } from '@react-types/menu'; import type { Alignment, @@ -43,7 +43,7 @@ export type MenuTriggerProps = { * @default true */ closeOnSelect?: boolean; -} & _MenuTriggerProps; +} & Omit; export type ActionMenuProps = { /** Whether the element should receive focus on render. */