Refactored SplitButton (breaking - more)

.changeset/chilled-hounds-laugh.md

@@ -0,0 +1,5 @@
+---
+'@leafygreen-ui/split-button': major
+---
+
+Update types to reflect what is actually being passed through to the underlying `Menu`

packages/split-button/src/Menu/Menu.tsx

@@ -1,5 +1,4 @@
 import React, {
-  MouseEvent,
   MouseEventHandler,
   useCallback,
   useMemo,
@@ -15,34 +14,55 @@ import { useDarkMode } from '@leafygreen-ui/leafygreen-provider';
 import { isComponentType, keyMap } from '@leafygreen-ui/lib';
 import { Menu as LGMenu } from '@leafygreen-ui/menu';
 
-import { MenuItemType } from '../SplitButton';
-
 import {
   triggerBaseStyles,
   triggerSizeStyles,
   triggerThemeStyles,
 } from './Menu.styles';
-import { MenuProps } from './Menu.types';
+import { ItemClickHandler, MenuProps } from './Menu.types';
+
+function hasOnClickHandler(
+  element: React.ReactElement<unknown>,
+): element is React.ReactElement<{
+  onClick: React.MouseEventHandler<unknown>;
+}> {
+  return (
+    typeof element.props === 'object' &&
+    element.props !== null &&
+    'onClick' in element.props &&
+    typeof element.props.onClick === 'function'
+  );
+}
+
+export const defaultItemClick: ItemClickHandler = (
+  event,
+  { close, element },
+) => {
+  close();
+  if (hasOnClickHandler(element)) {
+    element.props.onClick(event);
+  }
+};
 
 /**
  * @internal
  */
 export const Menu = ({
+  children,
   variant,
   disabled,
   align,
   justify,
   size,
   baseFontSize,
-  menuItems,
   containerRef,
   portalRef,
   id,
   onTriggerClick,
-  onChange,
   triggerAriaLabel,
   open: controlledOpen,
   setOpen: controlledSetOpen,
+  onItemClick,
   ...rest
 }: MenuProps) => {
   const { theme } = useDarkMode();
@@ -80,40 +100,19 @@ export const Menu = ({
   useEventListener('keydown', handleKeyDown, { enabled: open });
   useBackdropClick(handleClose, [buttonRef, menuRef], open);
 
-  const renderMenuItems = useMemo(() => {
-    const onMenuItemClick = (e: MouseEvent, menuItem: MenuItemType) => {
-      handleClose();
-      menuItem.props.onClick?.(e);
-      onChange?.(e);
-    };
-
-    const renderMenuItem = (menuItem: MenuItemType) => {
-      if (isComponentType(menuItem, 'MenuItem')) {
-        return React.cloneElement(menuItem, {
-          active: false,
-          key: `menuItem-${menuItem.key}`,
-          onClick: (e: MouseEvent) => onMenuItemClick(e, menuItem),
+  const enhancedChildren = useMemo(() => {
+    return React.Children.map(children, (child): React.ReactNode => {
+      if (isComponentType(child, 'MenuItem')) {
+        return React.cloneElement(child, {
+          onClick(event: React.MouseEvent<unknown>) {
+            onItemClick(event, { element: child, close: handleClose });
+          },
         });
       } else {
-        console.warn(
-          'Please use a LeafyGreen <MenuItem /> component. Received: ',
-          menuItem,
-        );
+        return child;
       }
-    };
-
-    if (Array.isArray(menuItems) && menuItems.length) {
-      return menuItems.map(
-        (menuItem: MenuItemType): MenuItemType | undefined => {
-          if (menuItem == null) {
-            return menuItem;
-          }
-
-          return renderMenuItem(menuItem);
-        },
-      );
-    }
-  }, [handleClose, menuItems, onChange]);
+    });
+  }, [handleClose, children, onItemClick]);
 
   return (
     <LGMenu
@@ -145,7 +144,7 @@ export const Menu = ({
         />
       }
     >
-      {renderMenuItems}
+      {enhancedChildren}
     </LGMenu>
   );
 };

packages/split-button/src/Menu/Menu.types.ts

@@ -4,9 +4,23 @@ import {
 } from '../SplitButton/SplitButton.types';
 
 export type MenuProps = Required<
-  Pick<SplitButtonProps, 'variant' | 'size' | 'disabled'>
+  Pick<SplitButtonProps, 'variant' | 'size' | 'disabled' | 'onItemClick'>
 > &
   Pick<SplitButtonProps, 'baseFontSize'> &
   SBMenuProps & {
     containerRef: React.RefObject<HTMLElement>;
   };
+
+export type ItemClickHandlerOptions = {
+  /**
+   * Closes the menu.
+   */
+  close(): void;
+
+  /**
+   * The element being clicked.
+   */
+  element: React.ReactElement<unknown>;
+};
+
+export type ItemClickHandler = (event: React.MouseEvent<unknown>, options: ItemClickHandlerOptions) => void;

packages/split-button/src/Menu/index.ts

@@ -1 +1 @@
-export { Menu } from './Menu';
+export { Menu, defaultItemClick } from './Menu';

packages/split-button/src/SplitButton.stories.tsx

@@ -30,12 +30,12 @@ const meta: StoryMetaType<typeof SplitButton> = {
         ...storybookExcludedControlParams,
         'as',
         'children',
-        'menuItems',
         'href',
         'type',
         'maxHeight',
         'open',
         'onTriggerClick',
+        'onItemClick',
         'triggerAriaLabel',
       ],
     },
@@ -74,7 +74,7 @@ const meta: StoryMetaType<typeof SplitButton> = {
     variant: Variant.Default,
     align: Align.Bottom,
     justify: Justify.End,
-    menuItems: [
+    children: [
       <MenuItem key="0" onClick={(event: MouseEvent) => console.log(event)}>
         Menu Item
       </MenuItem>,

packages/split-button/src/SplitButton/SplitButton.spec.tsx

@@ -33,7 +33,7 @@ const getMenuItems = (): MenuItemsType => {
 
 const defaultProps = {
   label: 'Button Label',
-  menuItems: getMenuItems(),
+  children: getMenuItems(),
 };
 
 function renderSplitButton(props = {}) {
@@ -216,16 +216,16 @@ describe('packages/split-button', () => {
   });
 
   describe('MenuItem', () => {
-    test('click triggers onChange callback', () => {
-      const onChange = jest.fn();
-      const { menuTrigger, getByTestId } = renderSplitButton({ onChange });
+    test('click triggers onItemClick callback', () => {
+      const onItemClick = jest.fn();
+      const { menuTrigger, getByTestId } = renderSplitButton({ onItemClick });
 
       userEvent.click(menuTrigger as HTMLElement);
 
       const menu = getByTestId(menuTestId);
       const options = globalGetAllByRole(menu, 'menuitem');
       userEvent.click(options[0]);
-      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onItemClick).toHaveBeenCalledTimes(1);
     });
   });
 
@@ -298,7 +298,7 @@ describe('packages/split-button', () => {
 
       test('Fires the click handler of the highlighted item', async () => {
         const { openMenu } = renderSplitButton({
-          menuItems,
+          children: menuItems,
         });
         const { menuItemElements } = await openMenu({
           withKeyboard: true,
@@ -315,7 +315,7 @@ describe('packages/split-button', () => {
         // https://github.com/testing-library/react-testing-library/issues/269#issuecomment-1453666401 - this needs v13 of testing-library
         // TODO: This is not triggered so the test fails
         const { openMenu, menuTrigger } = renderSplitButton({
-          menuItems,
+          children: menuItems,
         });
         const { menuEl, menuItemElements } = await openMenu();
         userEvent.type(menuItemElements?.[0]!, `{${key}}`);
@@ -335,7 +335,7 @@ describe('packages/split-button', () => {
 
       describe('Down arrow', () => {
         test('highlights the next option in the menu', async () => {
-          const { openMenu } = renderSplitButton({ menuItems });
+          const { openMenu } = renderSplitButton({ children: menuItems });
           const { menuEl, menuItemElements } = await openMenu({
             withKeyboard: true,
           });
@@ -346,7 +346,7 @@ describe('packages/split-button', () => {
         });
 
         test('cycles highlight to the top', async () => {
-          const { openMenu } = renderSplitButton({ menuItems });
+          const { openMenu } = renderSplitButton({ children: menuItems });
           const { menuEl, menuItemElements } = await openMenu({
             withKeyboard: true,
           });
@@ -362,7 +362,7 @@ describe('packages/split-button', () => {
 
       describe('Up arrow', () => {
         test('highlights the previous option in the menu', async () => {
-          const { openMenu } = renderSplitButton({ menuItems });
+          const { openMenu } = renderSplitButton({ children: menuItems });
           const { menuEl, menuItemElements } = await openMenu({
             withKeyboard: true,
           });
@@ -376,7 +376,7 @@ describe('packages/split-button', () => {
         });
 
         test('cycles highlight to the bottom', async () => {
-          const { openMenu } = renderSplitButton({ menuItems });
+          const { openMenu } = renderSplitButton({ children: menuItems });
           const { menuEl, menuItemElements } = await openMenu({
             withKeyboard: true,
           });
@@ -394,22 +394,19 @@ describe('packages/split-button', () => {
   describe('Types behave as expected', () => {
     test.skip('Accepts base props', () => {
       <>
-        <SplitButton label="label" menuItems={getMenuItems()} />
+        <SplitButton label="label" children={getMenuItems()} />
+        <SplitButton label="label">
+          <MenuItem key="0">Menu Item</MenuItem>
+          <MenuItem key="1" disabled>
+            Disabled Menu Item
+          </MenuItem>
+          <MenuItem key="2" description="I am also a description">
+            Menu Item With Description
+          </MenuItem>
+        </SplitButton>
         <SplitButton
           label="label"
-          menuItems={[
-            <MenuItem key="0">Menu Item</MenuItem>,
-            <MenuItem key="1" disabled>
-              Disabled Menu Item
-            </MenuItem>,
-            <MenuItem key="2" description="I am also a description">
-              , Menu Item With Description
-            </MenuItem>,
-          ]}
-        />
-        <SplitButton
-          label="label"
-          menuItems={getMenuItems()}
+          children={getMenuItems()}
           onClick={() => {}}
           disabled={true}
           size="default"
@@ -426,17 +423,15 @@ describe('packages/split-button', () => {
           open={false}
           triggerAriaLabel="im the trigger silly"
         />
-        {/* @ts-expect-error - expects label and menuItems*/}
+        {/* @ts-expect-error - expects label */}
         <SplitButton />
-        {/* @ts-expect-error - expects menuItems */}
-        <SplitButton label="label" />
         {/* @ts-expect-error - expects label */}
-        <SplitButton menuItems={getMenuItems()} />
+        <SplitButton children={getMenuItems()} />
       </>;
     });
 
     test.skip('Accepts string as `as` prop', () => {
-      <SplitButton as="p" label="label" menuItems={getMenuItems()} />;
+      <SplitButton as="p" label="label" children={getMenuItems()} />;
     });
 
     test.skip('Accepts component as `as` prop', () => {
@@ -448,7 +443,7 @@ describe('packages/split-button', () => {
           data-testid="split-button"
           as={As}
           label="label"
-          menuItems={getMenuItems()}
+          children={getMenuItems()}
         />,
       );
     });
@@ -459,25 +454,25 @@ describe('packages/split-button', () => {
       };
 
       <>
-        <SplitButton href="allowed" label="label" menuItems={getMenuItems()} />
+        <SplitButton href="allowed" label="label" children={getMenuItems()} />
         <SplitButton
           as="a"
           href="allowed"
           label="label"
-          menuItems={getMenuItems()}
+          children={getMenuItems()}
         />
         <SplitButton
           as="div"
           // @ts-expect-error - href not allowed when as is div
           href="string"
           label="label"
-          menuItems={getMenuItems()}
+          children={getMenuItems()}
         />
         <SplitButton
           as={AnchorLikeWrapper}
           href="string"
           label="label"
-          menuItems={getMenuItems()}
+          children={getMenuItems()}
         />
       </>;
     });