Fix <Menu> onAction being double called

Thinkmill · Oct 22, 2024 · 5a285f8 · 5a285f8
1 parent 1d77a3b
commit 5a285f8
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 31 deletions.
diff --git a/.changeset/gorgeous-cats-wave.md b/.changeset/gorgeous-cats-wave.md
@@ -0,0 +1,5 @@
+---
+'@keystatic/core': patch
+---
+
+Fix menus items like inserting/removing columns/rows in tables in editors and inserting custom components triggering twice when clicked
diff --git a/.changeset/khaki-elephants-cry.md b/.changeset/khaki-elephants-cry.md
@@ -0,0 +1,5 @@
+---
+'@keystar/ui': patch
+---
+
+Fix `Menu` `onAction` being double called
diff --git a/design-system/pkg/src/menu/Menu.tsx b/design-system/pkg/src/menu/Menu.tsx
@@ -36,24 +36,10 @@ function Menu<T extends object>(
     >
       {[...state.collection].map(item => {
         if (item.type === 'section') {
-          return (
-            <MenuSection
-              key={item.key}
-              item={item}
-              state={state}
-              onAction={completeProps.onAction}
-            />
-          );
+          return <MenuSection key={item.key} item={item} state={state} />;
         }
 
-        let menuItem = (
-          <MenuItem
-            key={item.key}
-            item={item}
-            state={state}
-            onAction={completeProps.onAction}
-          />
-        );
+        let menuItem = <MenuItem key={item.key} item={item} state={state} />;
 
         if (item.wrapper) {
           menuItem = item.wrapper(menuItem);

diff --git a/design-system/pkg/src/menu/MenuItem.tsx b/design-system/pkg/src/menu/MenuItem.tsx
@@ -4,7 +4,7 @@ import { useMenuItem } from '@react-aria/menu';
 import { mergeProps } from '@react-aria/utils';
 import { TreeState } from '@react-stately/tree';
 import { Node } from '@react-types/shared';
-import { Key, useRef } from 'react';
+import { useRef } from 'react';
 
 import { ListItem } from '@keystar/ui/listbox';
 import { Text } from '@keystar/ui/typography';
@@ -16,12 +16,11 @@ type MenuItemProps<T> = {
   item: Node<T>;
   state: TreeState<T>;
   isVirtualized?: boolean;
-  onAction?: (key: Key) => void;
 };
 
 /** @private */
 export function MenuItem<T>(props: MenuItemProps<T>) {
-  let { item, state, isVirtualized, onAction } = props;
+  let { item, state, isVirtualized } = props;
   let { onClose, closeOnSelect } = useMenuContext();
 
   let { rendered, key } = item;
@@ -40,7 +39,6 @@ export function MenuItem<T>(props: MenuItemProps<T>) {
         onClose,
         closeOnSelect,
         isVirtualized,
-        onAction,
       },
       state,
       ref

diff --git a/design-system/pkg/src/menu/MenuSection.tsx b/design-system/pkg/src/menu/MenuSection.tsx
@@ -3,7 +3,7 @@ import { useSeparator } from '@react-aria/separator';
 import { getChildNodes } from '@react-stately/collections';
 import { TreeState } from '@react-stately/tree';
 import { Node } from '@react-types/shared';
-import { Fragment, Key } from 'react';
+import { Fragment } from 'react';
 
 import { Divider } from '@keystar/ui/layout';
 import { css, tokenSchema } from '@keystar/ui/style';
@@ -14,12 +14,11 @@ import { MenuItem } from './MenuItem';
 interface MenuSectionProps<T> {
   item: Node<T>;
   state: TreeState<T>;
-  onAction?: (key: Key) => void;
 }
 
 /** @private */
 export function MenuSection<T>(props: MenuSectionProps<T>) {
-  let { item, state, onAction } = props;
+  let { item, state } = props;
   let { itemProps, headingProps, groupProps } = useMenuSection({
     heading: item.rendered,
     'aria-label': item['aria-label'],
@@ -50,14 +49,7 @@ export function MenuSection<T>(props: MenuSectionProps<T>) {
         )}
         <div {...groupProps}>
           {[...getChildNodes(item, state.collection)].map(node => {
-            let item = (
-              <MenuItem
-                key={node.key}
-                item={node}
-                state={state}
-                onAction={onAction}
-              />
-            );
+            let item = <MenuItem key={node.key} item={node} state={state} />;
 
             if (node.wrapper) {
               item = node.wrapper(item);

diff --git a/design-system/pkg/src/menu/test/Menu.test.tsx b/design-system/pkg/src/menu/test/Menu.test.tsx
@@ -567,14 +567,17 @@ describe('menu/Menu', () => {
 
       firePress(item1);
       expect(onAction).toHaveBeenCalledWith('One');
+      expect(onAction).toHaveBeenCalledTimes(1);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
 
       firePress(item2);
       expect(onAction).toHaveBeenCalledWith('Two');
+      expect(onAction).toHaveBeenCalledTimes(2);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
 
       firePress(item3);
       expect(onAction).toHaveBeenCalledWith('Three');
+      expect(onAction).toHaveBeenCalledTimes(3);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
     });
 
@@ -607,14 +610,17 @@ describe('menu/Menu', () => {
 
       firePress(item1);
       expect(onAction).toHaveBeenCalledWith('One');
+      expect(onAction).toHaveBeenCalledTimes(1);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
 
       firePress(item2);
       expect(onAction).toHaveBeenCalledWith('Two');
+      expect(onAction).toHaveBeenCalledTimes(2);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
 
       firePress(item3);
       expect(onAction).toHaveBeenCalledWith('Three');
+      expect(onAction).toHaveBeenCalledTimes(3);
       expect(onSelectionChange).toHaveBeenCalledTimes(0);
     });
   });