Revert "fix: prevent closing menu when event.preventDefault() is ca…

…lled on `ActionList.Item`'s `onSelect` handler by @gr2m (#3346)" This reverts commit 6304923.
primer · Jun 12, 2023 · c6b9e25 · c6b9e25
1 parent 78b01ad
commit c6b9e25
Show file tree

Hide file tree

Showing 6 changed files with 13 additions and 59 deletions.
diff --git a/.changeset/brave-cherries-march.md b/.changeset/brave-cherries-march.md
diff --git a/generated/components.json b/generated/components.json
@@ -163,7 +163,7 @@
               "name": "onSelect",
               "type": "(event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => void",
               "defaultValue": "",
-              "description": "Callback that is called when the item is selected using either the mouse or keyboard. `event.preventDefault()` will prevent a menu from closing when within an `<ActionMenu />`"
+              "description": "Callback that is called when the item is selected using either the mouse or keyboard."
             },
             {
               "name": "selected",

diff --git a/src/ActionList/ActionList.docs.json b/src/ActionList/ActionList.docs.json
@@ -62,7 +62,7 @@
           "name": "onSelect",
           "type": "(event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => void",
           "defaultValue": "",
-          "description": "Callback that is called when the item is selected using either the mouse or keyboard. `event.preventDefault()` will prevent a menu from closing when within an `<ActionMenu />`"
+          "description": "Callback that is called when the item is selected using either the mouse or keyboard."
         },
         {
           "name": "selected",

diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx
@@ -24,7 +24,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       disabled = false,
       selected = undefined,
       active = false,
-      onSelect: onSelectUser,
+      onSelect,
       sx: sxProp = defaultSxProp,
       id,
       role,
@@ -42,19 +42,6 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext)
     const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
 
-    const onSelect = React.useCallback(
-      (
-        event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>,
-        // eslint-disable-next-line @typescript-eslint/ban-types
-        afterSelect?: Function,
-      ) => {
-        if (typeof onSelectUser === 'function') onSelectUser(event)
-        if (event.defaultPrevented) return
-        if (typeof afterSelect === 'function') afterSelect()
-      },
-      [onSelectUser],
-    )
-
     const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant
       ? groupSelectionVariant
       : listSelectionVariant
@@ -162,16 +149,22 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const clickHandler = React.useCallback(
       (event: React.MouseEvent<HTMLLIElement>) => {
         if (disabled) return
-        onSelect(event, afterSelect)
+        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],
     )
 
     const keyPressHandler = React.useCallback(
       (event: React.KeyboardEvent<HTMLLIElement>) => {
         if (disabled) return
-        if ([' ', 'Enter'].includes(event.key)) {
-          onSelect(event, afterSelect)
+        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()
         }
       },
       [onSelect, disabled, afterSelect],

diff --git a/src/ActionMenu/ActionMenu.examples.stories.tsx b/src/ActionMenu/ActionMenu.examples.stories.tsx
@@ -11,8 +11,6 @@ import {
   NumberIcon,
   CalendarIcon,
   XIcon,
-  CheckIcon,
-  CopyIcon,
 } from '@primer/octicons-react'
 
 export default {
@@ -275,35 +273,3 @@ export const MultipleSections = () => {
     </ActionMenu>
   )
 }
-
-export const DelayedMenuClose = () => {
-  const [open, setOpen] = React.useState(false)
-  const [copyLinkSuccess, setCopyLinkSuccess] = React.useState(false)
-  const onSelect = (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => {
-    event.preventDefault()
-
-    setCopyLinkSuccess(true)
-    setTimeout(() => {
-      setOpen(false)
-      setCopyLinkSuccess(false)
-    }, 700)
-  }
-
-  return (
-    <>
-      <h1>Delayed Menu Close</h1>
-
-      <ActionMenu open={open} onOpenChange={setOpen}>
-        <ActionMenu.Button>Anchor</ActionMenu.Button>
-        <ActionMenu.Overlay>
-          <ActionList>
-            <ActionList.Item onSelect={onSelect}>
-              <ActionList.LeadingVisual>{copyLinkSuccess ? <CheckIcon /> : <CopyIcon />}</ActionList.LeadingVisual>
-              {copyLinkSuccess ? 'Copied!' : 'Copy link'}
-            </ActionList.Item>
-          </ActionList>
-        </ActionMenu.Overlay>
-      </ActionMenu>
-    </>
-  )
-}
diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx
@@ -22,7 +22,7 @@ function Example(): JSX.Element {
                 <ActionList.Divider />
                 <ActionList.Item>Copy link</ActionList.Item>
                 <ActionList.Item>Edit file</ActionList.Item>
-                <ActionList.Item variant="danger" onSelect={event => event.preventDefault()}>
+                <ActionList.Item variant="danger" onClick={event => event.preventDefault()}>
                   Delete file
                 </ActionList.Item>
                 <ActionList.LinkItem href="//github.com" title="anchor" aria-keyshortcuts="s">