From 64b1ecfe9a4d7aadbd27bcf4af2401adae52fc94 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 23 May 2023 16:20:55 +0200 Subject: [PATCH 1/3] Check defaultPrevented before afterSelect --- .../actionlist-check-event-after-onselect.md | 5 +++ src/ActionList/Item.tsx | 37 +++++++++---------- .../ActionMenu.examples.stories.tsx | 35 ++++++++++++++++++ src/__tests__/ActionMenu.test.tsx | 2 +- 4 files changed, 59 insertions(+), 20 deletions(-) create mode 100644 .changeset/actionlist-check-event-after-onselect.md diff --git a/.changeset/actionlist-check-event-after-onselect.md b/.changeset/actionlist-check-event-after-onselect.md new file mode 100644 index 00000000000..d347b7a9a3b --- /dev/null +++ b/.changeset/actionlist-check-event-after-onselect.md @@ -0,0 +1,5 @@ +--- +"@primer/react": minor +--- + +ActionMenu: Calling `event.preventDefault` inside `onSelect` of `ActionList.Item` will prevent the default behavior of closing the menu diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 8064ff2f1c1..2564d072fc5 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -146,30 +146,29 @@ export const Item = React.forwardRef( ...(active ? activeStyles : {}), } - const clickHandler = React.useCallback( - (event: React.MouseEvent) => { + const internalOnSelect = React.useCallback( + (event: React.MouseEvent | React.KeyboardEvent) => { if (disabled) return - if (!event.defaultPrevented) { - if (typeof onSelect === 'function') onSelect(event) - // if this Item is inside a Menu, close the Menu - if (typeof afterSelect === 'function') afterSelect() - } - }, - [onSelect, disabled, afterSelect], - ) + if (event.defaultPrevented) return - const keyPressHandler = React.useCallback( - (event: React.KeyboardEvent) => { - if (disabled) return - if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) { - if (typeof onSelect === 'function') onSelect(event) - // if this Item is inside a Menu, close the Menu - if (typeof afterSelect === 'function') afterSelect() - } + if (typeof onSelect === 'function') onSelect(event) + + // typescript does not know onSelect can call event.preventDefault, so this check is valid + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (event.defaultPrevented) return + + // if this Item is inside a Menu, close the Menu + if (typeof afterSelect === 'function') afterSelect() }, - [onSelect, disabled, afterSelect], + [disabled, onSelect, afterSelect], ) + const clickHandler = internalOnSelect + + const keyPressHandler = (event: React.KeyboardEvent) => { + if ([' ', 'Enter'].includes(event.key)) internalOnSelect(event) + } + // use props.id if provided, otherwise generate one. const labelId = useId(id) const inlineDescriptionId = useId(id && `${id}--inline-description`) diff --git a/src/ActionMenu/ActionMenu.examples.stories.tsx b/src/ActionMenu/ActionMenu.examples.stories.tsx index 5632bfe4ae0..99eda066f50 100644 --- a/src/ActionMenu/ActionMenu.examples.stories.tsx +++ b/src/ActionMenu/ActionMenu.examples.stories.tsx @@ -11,6 +11,8 @@ import { NumberIcon, CalendarIcon, XIcon, + CheckIcon, + CopyIcon, } from '@primer/octicons-react' export default { @@ -282,3 +284,36 @@ export const MultipleSections = () => { ) } + +export const DelayedMenuClose = () => { + const [open, setOpen] = React.useState(false) + const [copyLinkSuccess, setCopyLinkSuccess] = React.useState(false) + const onSelect = (event: React.MouseEvent | React.KeyboardEvent) => { + // don't close the menu + event.preventDefault() + + setCopyLinkSuccess(true) + setTimeout(() => { + setOpen(false) + setCopyLinkSuccess(false) + }, 1000) + } + + return ( + <> +

Delayed Menu Close

+ + + Anchor + + + + {copyLinkSuccess ? : } + {copyLinkSuccess ? 'Copied!' : 'Copy link'} + + + + + + ) +} diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx index f2b41c86df1..bf33d15c491 100644 --- a/src/__tests__/ActionMenu.test.tsx +++ b/src/__tests__/ActionMenu.test.tsx @@ -22,7 +22,7 @@ function Example(): JSX.Element { Copy link Edit file - event.preventDefault()}> + event.preventDefault()}> Delete file From 8f9e906c8d1599190062646e3e825c45cb1447b5 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 23 May 2023 16:25:50 +0200 Subject: [PATCH 2/3] change variables to be more generic --- src/ActionMenu/ActionMenu.examples.stories.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ActionMenu/ActionMenu.examples.stories.tsx b/src/ActionMenu/ActionMenu.examples.stories.tsx index 99eda066f50..7e842cc736c 100644 --- a/src/ActionMenu/ActionMenu.examples.stories.tsx +++ b/src/ActionMenu/ActionMenu.examples.stories.tsx @@ -287,15 +287,15 @@ export const MultipleSections = () => { export const DelayedMenuClose = () => { const [open, setOpen] = React.useState(false) - const [copyLinkSuccess, setCopyLinkSuccess] = React.useState(false) + const [copied, setCopied] = React.useState(false) const onSelect = (event: React.MouseEvent | React.KeyboardEvent) => { // don't close the menu event.preventDefault() - setCopyLinkSuccess(true) + setCopied(true) setTimeout(() => { setOpen(false) - setCopyLinkSuccess(false) + setCopied(false) }, 1000) } @@ -308,8 +308,8 @@ export const DelayedMenuClose = () => { - {copyLinkSuccess ? : } - {copyLinkSuccess ? 'Copied!' : 'Copy link'} + {copied ? : } + {copied ? 'Copied!' : 'Copy link'} From 3149d5bbb15a73305839d9abff1588333f8b8773 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 23 May 2023 16:40:48 +0200 Subject: [PATCH 3/3] clean up type for onSelect --- src/ActionMenu/ActionMenu.examples.stories.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ActionMenu/ActionMenu.examples.stories.tsx b/src/ActionMenu/ActionMenu.examples.stories.tsx index 7e842cc736c..9fe1a01e166 100644 --- a/src/ActionMenu/ActionMenu.examples.stories.tsx +++ b/src/ActionMenu/ActionMenu.examples.stories.tsx @@ -1,5 +1,5 @@ import React from 'react' -import {Box, ActionMenu, ActionList, Button, IconButton} from '../' +import {Box, ActionMenu, ActionList, Button, IconButton, ActionListItemProps} from '../' import { GearIcon, MilestoneIcon, @@ -288,8 +288,8 @@ export const MultipleSections = () => { export const DelayedMenuClose = () => { const [open, setOpen] = React.useState(false) const [copied, setCopied] = React.useState(false) - const onSelect = (event: React.MouseEvent | React.KeyboardEvent) => { - // don't close the menu + const onSelect: ActionListItemProps['onSelect'] = event => { + // prevent default behavior of closing menu event.preventDefault() setCopied(true)