refactor: use item groups instead of React elements, use wrapper, keep menu state consistent

gagik · gagik · commit a54c7384b434 · 2025-05-23T13:50:50.000+02:00
The refactor is meant to make the Leafygreen integration more straightforward:
1. We stick to item groups and instead have a single wrapper to handle any rendering differences between groups. This allows the wrapper to always have context of all items when rendering which is useful when inserting menu seperators in Leafygreen. Also encourages consistent UI (while allowing per-case customization if needed at wrapper-level). We could introduce itemWrappers instead of itemGroups but having one wrapper handling all seems cleaner to me.
2. More of the responsibility is moved to a parent wrapper component that will house the context menu. This allows us to standardize the right click menu and make better use of Leafygreen's menu component including its click handling (which has been removed from the context menu library).
3. Menu state (i.e. position) is now preserved even closed; this is useful for leafygreen's menu to animate in the same position instead of losing the position all together.
diff --git a/packages/compass-context-menu/src/context-menu-content.ts b/packages/compass-context-menu/src/context-menu-content.ts
@@ -1,23 +1,24 @@
+import type { ContextMenuItemGroup } from './types';
+
 const CONTEXT_MENUS_SYMBOL = Symbol('context_menus');
 
 export type EnhancedMouseEvent = MouseEvent & {
-  [CONTEXT_MENUS_SYMBOL]?: React.ComponentType[];
+  [CONTEXT_MENUS_SYMBOL]?: ContextMenuItemGroup[];
 };
 
 export function getContextMenuContent(
   event: EnhancedMouseEvent
-): React.ComponentType[] {
+): ContextMenuItemGroup[] {
   return event[CONTEXT_MENUS_SYMBOL] ?? [];
 }
 
 export function appendContextMenuContent(
   event: EnhancedMouseEvent,
-  content: React.ComponentType
+  content: ContextMenuItemGroup
 ) {
   // Initialize if not already patched
-  if (event[CONTEXT_MENUS_SYMBOL] === undefined) {
-    event[CONTEXT_MENUS_SYMBOL] = [content];
-    return;
+  if (!event[CONTEXT_MENUS_SYMBOL]) {
+    event[CONTEXT_MENUS_SYMBOL] = [];
   }
   event[CONTEXT_MENUS_SYMBOL].push(content);
 }
diff --git a/packages/compass-context-menu/src/context-menu-provider.tsx b/packages/compass-context-menu/src/context-menu-provider.tsx
@@ -5,29 +5,50 @@ import React, {
   useMemo,
   createContext,
 } from 'react';
-import type { ContextMenuContext, MenuState } from './types';
-import { ContextMenu } from './context-menu';
+import type { ContextMenuContext, ContextMenuState } from './types';
 import type { EnhancedMouseEvent } from './context-menu-content';
 import { getContextMenuContent } from './context-menu-content';
 
 export const Context = createContext<ContextMenuContext | null>(null);
 
 export function ContextMenuProvider({
   children,
+  wrapper,
 }: {
   children: React.ReactNode;
+  wrapper: React.ComponentType<{
+    menu: ContextMenuState & { close: () => void };
+  }>;
 }) {
-  const [menu, setMenu] = useState<MenuState>({ isOpen: false });
-  const close = useCallback(() => setMenu({ isOpen: false }), [setMenu]);
+  const [menu, setMenu] = useState<ContextMenuState>({
+    isOpen: false,
+    itemGroups: [],
+    position: { x: 0, y: 0 },
+  });
+  const close = useCallback(() => setMenu({ ...menu, isOpen: false }), [menu]);
+
+  const handleClosingEvent = useCallback(
+    (event: Event) => {
+      if (!event.defaultPrevented) {
+        setMenu({ ...menu, isOpen: false });
+      }
+    },
+    [menu]
+  );
 
   useEffect(() => {
     function handleContextMenu(event: MouseEvent) {
       event.preventDefault();
+
+      const itemGroups = getContextMenuContent(event as EnhancedMouseEvent);
+
+      if (itemGroups.length === 0) {
+        return;
+      }
+
       setMenu({
         isOpen: true,
-        children: getContextMenuContent(event as EnhancedMouseEvent).map(
-          (Content, index) => <Content key={`menu-content-${index}`} />
-        ),
+        itemGroups,
         position: {
           // TODO: Fix handling offset while scrolling
           x: event.clientX,
@@ -36,22 +57,14 @@ export function ContextMenuProvider({
       });
     }
 
-    function handleClosingEvent(event: Event) {
-      if (!event.defaultPrevented) {
-        setMenu({ isOpen: false });
-      }
-    }
-
     document.addEventListener('contextmenu', handleContextMenu);
-    document.addEventListener('click', handleClosingEvent);
     window.addEventListener('resize', handleClosingEvent);
 
     return () => {
       document.removeEventListener('contextmenu', handleContextMenu);
-      document.removeEventListener('click', handleClosingEvent);
       window.removeEventListener('resize', handleClosingEvent);
     };
-  }, [setMenu]);
+  }, [handleClosingEvent]);
 
   const value = useMemo(
     () => ({
@@ -60,12 +73,12 @@ export function ContextMenuProvider({
     [close]
   );
 
+  const Wrapper = wrapper ?? React.Fragment;
+
   return (
-    <>
-      <Context.Provider value={value}>{children}</Context.Provider>
-      {menu.isOpen && (
-        <ContextMenu position={menu.position}>{menu.children}</ContextMenu>
-      )}
-    </>
+    <Context.Provider value={value}>
+      {children}
+      <Wrapper menu={{ ...menu, close }} />
+    </Context.Provider>
   );
 }
diff --git a/packages/compass-context-menu/src/index.ts b/packages/compass-context-menu/src/index.ts
@@ -1,2 +1,7 @@
 export { useContextMenu } from './use-context-menu';
-export type { ContextMenuItem } from './types';
+export { ContextMenuProvider } from './context-menu-provider';
+export type {
+  ContextMenuItem,
+  ContextMenuItemGroup,
+  ContextMenuWrapperProps,
+} from './types';
diff --git a/packages/compass-context-menu/src/types.ts b/packages/compass-context-menu/src/types.ts
@@ -1,15 +1,20 @@
-export type MenuState =
-  | {
-      isOpen: false;
-    }
-  | {
-      isOpen: true;
-      children: React.ReactNode;
-      position: {
-        x: number;
-        y: number;
-      };
-    };
+export interface ContextMenuItemGroup {
+  items: ContextMenuItem[];
+  originListener: (event: MouseEvent) => void;
+}
+
+export type ContextMenuState = {
+  isOpen: boolean;
+  itemGroups: ContextMenuItemGroup[];
+  position: {
+    x: number;
+    y: number;
+  };
+};
+
+export type ContextMenuWrapperProps = {
+  menu: ContextMenuState & { close: () => void };
+};
 
 export type ContextMenuContext = {
   close(): void;
diff --git a/packages/compass-context-menu/src/use-context-menu.spec.tsx b/packages/compass-context-menu/src/use-context-menu.spec.tsx
@@ -4,27 +4,29 @@ import { expect } from 'chai';
 import sinon from 'sinon';
 import { useContextMenu } from './use-context-menu';
 import { ContextMenuProvider } from './context-menu-provider';
-import type { ContextMenuItem } from './types';
+import type { ContextMenuItem, ContextMenuWrapperProps } from './types';
 
 describe('useContextMenu', function () {
-  const TestMenu: React.FC<{ items: ContextMenuItem[] }> = ({ items }) => (
+  const TestMenu: React.FC<ContextMenuWrapperProps> = ({ menu }) => (
     <div data-testid="test-menu">
-      {items.map((item, idx) => (
-        <div
-          key={idx}
-          data-testid={`menu-item-${item.label}`}
-          role="menuitem"
-          tabIndex={0}
-          onClick={(event) => item.onAction?.(event)}
-          onKeyDown={(event) => {
-            if (event.key === 'Enter') {
-              item.onAction?.(event);
-            }
-          }}
-        >
-          {item.label}
-        </div>
-      ))}
+      {menu.itemGroups.flatMap((group, groupIdx) =>
+        group.items.map((item, idx) => (
+          <div
+            key={`${groupIdx}-${idx}`}
+            data-testid={`menu-item-${item.label}`}
+            role="menuitem"
+            tabIndex={0}
+            onClick={(event) => item.onAction?.(event)}
+            onKeyDown={(event) => {
+              if (event.key === 'Enter') {
+                item.onAction?.(event);
+              }
+            }}
+          >
+            {item.label}
+          </div>
+        ))
+      )}
     </div>
   );
 
@@ -33,9 +35,9 @@ describe('useContextMenu', function () {
     onAction,
   }: {
     onRegister?: (ref: any) => void;
-    onAction?: (id) => void;
+    onAction?: (id: number) => void;
   }) => {
-    const contextMenu = useContextMenu({ Menu: TestMenu });
+    const contextMenu = useContextMenu();
     const items: ContextMenuItem[] = [
       {
         label: 'Test Item',
@@ -55,15 +57,14 @@ describe('useContextMenu', function () {
     );
   };
 
-  // Add new test components for nested context menu scenario
   const ParentComponent = ({
     onAction,
     children,
   }: {
     onAction?: (id: number) => void;
     children?: React.ReactNode;
   }) => {
-    const contextMenu = useContextMenu({ Menu: TestMenu });
+    const contextMenu = useContextMenu();
     const parentItems: ContextMenuItem[] = [
       {
         label: 'Parent Item 1',
@@ -89,7 +90,7 @@ describe('useContextMenu', function () {
   }: {
     onAction?: (id: number) => void;
   }) => {
-    const contextMenu = useContextMenu({ Menu: TestMenu });
+    const contextMenu = useContextMenu();
     const childItems: ContextMenuItem[] = [
       {
         label: 'Child Item 1',
@@ -135,7 +136,7 @@ describe('useContextMenu', function () {
 
     it('renders without error', function () {
       render(
-        <ContextMenuProvider>
+        <ContextMenuProvider wrapper={TestMenu}>
           <TestComponent />
         </ContextMenuProvider>
       );
@@ -147,7 +148,7 @@ describe('useContextMenu', function () {
       const onRegister = sinon.spy();
 
       render(
-        <ContextMenuProvider>
+        <ContextMenuProvider wrapper={TestMenu}>
           <TestComponent onRegister={onRegister} />
         </ContextMenuProvider>
       );
@@ -158,7 +159,7 @@ describe('useContextMenu', function () {
 
     it('shows context menu on right click', function () {
       render(
-        <ContextMenuProvider>
+        <ContextMenuProvider wrapper={TestMenu}>
           <TestComponent />
         </ContextMenuProvider>
       );
@@ -173,7 +174,7 @@ describe('useContextMenu', function () {
     describe('with nested context menus', function () {
       it('shows only parent items when right clicking parent area', function () {
         render(
-          <ContextMenuProvider>
+          <ContextMenuProvider wrapper={TestMenu}>
             <ParentComponent />
           </ContextMenuProvider>
         );
@@ -192,7 +193,7 @@ describe('useContextMenu', function () {
 
       it('shows both parent and child items when right clicking child area', function () {
         render(
-          <ContextMenuProvider>
+          <ContextMenuProvider wrapper={TestMenu}>
             <ParentComponent>
               <ChildComponent />
             </ParentComponent>
@@ -214,7 +215,7 @@ describe('useContextMenu', function () {
         const childOnAction = sinon.spy();
 
         render(
-          <ContextMenuProvider>
+          <ContextMenuProvider wrapper={TestMenu}>
             <ParentComponent onAction={parentOnAction}>
               <ChildComponent onAction={childOnAction} />
             </ParentComponent>
@@ -237,7 +238,7 @@ describe('useContextMenu', function () {
         const childOnAction = sinon.spy();
 
         render(
-          <ContextMenuProvider>
+          <ContextMenuProvider wrapper={TestMenu}>
             <ParentComponent onAction={parentOnAction}>
               <ChildComponent onAction={childOnAction} />
             </ParentComponent>
diff --git a/packages/compass-context-menu/src/use-context-menu.tsx b/packages/compass-context-menu/src/use-context-menu.tsx
