From 7ce8d1cb84e4be7a2a66facdb7a07be74e8c4c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Tue, 17 Dec 2024 12:36:51 +0100 Subject: [PATCH 1/4] Use Pick instead of Omit for MenuProps --- .../src/SplitButton/SplitButton.types.ts | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/split-button/src/SplitButton/SplitButton.types.ts b/packages/split-button/src/SplitButton/SplitButton.types.ts index 09e891c4e2..728a22ee0f 100644 --- a/packages/split-button/src/SplitButton/SplitButton.types.ts +++ b/packages/split-button/src/SplitButton/SplitButton.types.ts @@ -53,18 +53,22 @@ type ButtonProps = Omit< 'rightGlyph' | 'href' | 'as' | 'variant' >; -export type SelectedMenuProps = Omit< +export type SelectedMenuProps = Pick< ImportedMenuProps, - | 'children' - | 'trigger' - | 'shouldClose' | 'darkMode' - | 'onClick' + | 'maxHeight' + | 'adjustOnMutation' + | 'popoverZIndex' + | 'renderMode' + | 'portalClassName' + | 'portalContainer' + | 'portalRef' + | 'scrollContainer' | 'align' | 'justify' - | 'className' - | 'refEl' - | 'spacing' + | 'id' + | 'open' + | 'setOpen' >; export interface MenuProps extends SelectedMenuProps { From e720432945177fb1a9a2fe901e21fa114d4272fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Tue, 17 Dec 2024 12:44:45 +0100 Subject: [PATCH 2/4] Add changeset --- .changeset/chilled-hounds-laugh.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/chilled-hounds-laugh.md diff --git a/.changeset/chilled-hounds-laugh.md b/.changeset/chilled-hounds-laugh.md new file mode 100644 index 0000000000..a8d1f93e1c --- /dev/null +++ b/.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` From 1e19deac8fd2a0edd4f81b81a9710d5ef1e6f146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Tue, 17 Dec 2024 22:05:28 +0100 Subject: [PATCH 3/4] Pass children to menu and expose onItemClick instead of onChange --- packages/split-button/src/Menu/Menu.tsx | 75 +++++++++---------- packages/split-button/src/Menu/Menu.types.ts | 16 +++- packages/split-button/src/Menu/index.ts | 2 +- .../split-button/src/SplitButton.stories.tsx | 4 +- .../src/SplitButton/SplitButton.spec.tsx | 65 ++++++++-------- .../src/SplitButton/SplitButton.tsx | 14 ++-- .../src/SplitButton/SplitButton.types.ts | 31 +++----- 7 files changed, 101 insertions(+), 106 deletions(-) diff --git a/packages/split-button/src/Menu/Menu.tsx b/packages/split-button/src/Menu/Menu.tsx index ec1b5e97cd..4d9ad67d18 100644 --- a/packages/split-button/src/Menu/Menu.tsx +++ b/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, +): element is React.ReactElement<{ + onClick: React.MouseEventHandler; +}> { + 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) { + onItemClick(event, { element: child, close: handleClose }); + }, }); } else { - console.warn( - 'Please use a LeafyGreen 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 ( } > - {renderMenuItems} + {enhancedChildren} ); }; diff --git a/packages/split-button/src/Menu/Menu.types.ts b/packages/split-button/src/Menu/Menu.types.ts index 3d2aa27abf..ae41f13dfe 100644 --- a/packages/split-button/src/Menu/Menu.types.ts +++ b/packages/split-button/src/Menu/Menu.types.ts @@ -4,9 +4,23 @@ import { } from '../SplitButton/SplitButton.types'; export type MenuProps = Required< - Pick + Pick > & Pick & SBMenuProps & { containerRef: React.RefObject; }; + +export type ItemClickHandlerOptions = { + /** + * Closes the menu. + */ + close(): void; + + /** + * The element being clicked. + */ + element: React.ReactElement; +}; + +export type ItemClickHandler = (event: React.MouseEvent, options: ItemClickHandlerOptions) => void; diff --git a/packages/split-button/src/Menu/index.ts b/packages/split-button/src/Menu/index.ts index 81b93712f1..7907da421f 100644 --- a/packages/split-button/src/Menu/index.ts +++ b/packages/split-button/src/Menu/index.ts @@ -1 +1 @@ -export { Menu } from './Menu'; +export { Menu, defaultItemClick } from './Menu'; diff --git a/packages/split-button/src/SplitButton.stories.tsx b/packages/split-button/src/SplitButton.stories.tsx index c97d593d37..356b3d24df 100644 --- a/packages/split-button/src/SplitButton.stories.tsx +++ b/packages/split-button/src/SplitButton.stories.tsx @@ -30,12 +30,12 @@ const meta: StoryMetaType = { ...storybookExcludedControlParams, 'as', 'children', - 'menuItems', 'href', 'type', 'maxHeight', 'open', 'onTriggerClick', + 'onItemClick', 'triggerAriaLabel', ], }, @@ -74,7 +74,7 @@ const meta: StoryMetaType = { variant: Variant.Default, align: Align.Bottom, justify: Justify.End, - menuItems: [ + children: [ console.log(event)}> Menu Item , diff --git a/packages/split-button/src/SplitButton/SplitButton.spec.tsx b/packages/split-button/src/SplitButton/SplitButton.spec.tsx index 06b441a764..28601ad57d 100644 --- a/packages/split-button/src/SplitButton/SplitButton.spec.tsx +++ b/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', () => { <> - + + + Menu Item + + Disabled Menu Item + + + Menu Item With Description + + Menu Item, - - Disabled Menu Item - , - - , Menu Item With Description - , - ]} - /> - {}} 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 */} - {/* @ts-expect-error - expects menuItems */} - {/* @ts-expect-error - expects label */} - + ; }); test.skip('Accepts string as `as` prop', () => { - ; + ; }); 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', () => { }; <> - + ; }); diff --git a/packages/split-button/src/SplitButton/SplitButton.tsx b/packages/split-button/src/SplitButton/SplitButton.tsx index c501a85580..f38b4da8b9 100644 --- a/packages/split-button/src/SplitButton/SplitButton.tsx +++ b/packages/split-button/src/SplitButton/SplitButton.tsx @@ -14,7 +14,7 @@ import { import { RenderMode } from '@leafygreen-ui/popover'; import { BaseFontSize } from '@leafygreen-ui/tokens'; -import { Menu } from '../Menu'; +import { Menu, defaultItemClick } from '../Menu'; import { buttonBaseStyles, @@ -26,6 +26,7 @@ import { Align, Justify, SplitButtonProps, Variant } from './SplitButton.types'; export const SplitButton = InferredPolymorphic( ( { + children, darkMode: darkModeProp, variant = Variant.Default, type = 'button', @@ -33,7 +34,6 @@ export const SplitButton = InferredPolymorphic( justify = Justify.End, size = Size.Default, disabled = false, - menuItems = [], as, baseFontSize, label, @@ -49,8 +49,8 @@ export const SplitButton = InferredPolymorphic( open, setOpen, onTriggerClick, + onItemClick = defaultItemClick, triggerAriaLabel, - onChange, ...rest }, ref: React.Ref, @@ -101,15 +101,16 @@ export const SplitButton = InferredPolymorphic( align={align} justify={justify} containerRef={containerRef} - menuItems={menuItems} id={menuId} disabled={disabled} open={open} setOpen={setOpen} onTriggerClick={onTriggerClick} + onItemClick={onItemClick} triggerAriaLabel={triggerAriaLabel} - onChange={onChange} - /> + > + {children} + ); @@ -126,7 +127,6 @@ SplitButton.propTypes = { justify: PropTypes.oneOf(Object.values(Justify)), variant: PropTypes.oneOf(Object.values(Variant)), label: PropTypes.string.isRequired, - menuItems: PropTypes.arrayOf(PropTypes.element).isRequired, baseFontSize: PropTypes.oneOf(Object.values(BaseFontSize)), disabled: PropTypes.bool, leftGlyph: PropTypes.element, diff --git a/packages/split-button/src/SplitButton/SplitButton.types.ts b/packages/split-button/src/SplitButton/SplitButton.types.ts index 728a22ee0f..fe61ced90e 100644 --- a/packages/split-button/src/SplitButton/SplitButton.types.ts +++ b/packages/split-button/src/SplitButton/SplitButton.types.ts @@ -17,6 +17,7 @@ import { Align as ImportedAlign, Justify as ImportedJustify, } from '@leafygreen-ui/popover'; +import type { ItemClickHandler } from '../Menu/Menu.types'; export type MenuItemType = ReactElement< InferredPolymorphicProps @@ -50,11 +51,12 @@ export type Justify = (typeof Justify)[keyof typeof Justify]; // https://jira.mongodb.org/browse/LG-3260 type ButtonProps = Omit< ImportedButtonProps, - 'rightGlyph' | 'href' | 'as' | 'variant' + 'children' | 'rightGlyph' | 'href' | 'as' | 'variant' >; export type SelectedMenuProps = Pick< ImportedMenuProps, + | 'children' | 'darkMode' | 'maxHeight' | 'adjustOnMutation' @@ -86,43 +88,28 @@ export interface MenuProps extends SelectedMenuProps { */ justify?: Justify; - /** - * The menu items to appear in the menu dropdown. Must be an array of ``. - * - * ```js - * [ - * {}}> Menu Item, - * Disabled Menu Item, - * , - * Menu Item With Description - * - * ] - * ``` - * - * @type Array - */ - menuItems: MenuItemsType; - /** * Callback fired when the trigger is clicked. */ onTriggerClick?: MouseEventHandler; /** - * aria-label for the menu trigger button. + * Callback called when an item is clicked, with the click event and an options object. + * Provide an implementation to avoid the default behavior of the menu closing when an item is clicked. */ - triggerAriaLabel?: string; + onItemClick?: ItemClickHandler; /** - * Callback fired when a menuItem is clicked. + * aria-label for the menu trigger button. */ - onChange?: MouseEventHandler; + triggerAriaLabel?: string; } export interface SplitButtonProps extends DarkModeProps, ButtonProps, MenuProps { + /** * Sets the variant for both Buttons. * From c0ff482e08afc7d5b96819a43fe76d056853ee43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Wed, 18 Dec 2024 09:42:23 +0100 Subject: [PATCH 4/4] fixup! Pass children to menu and expose onItemClick instead of onChange Delete unused types --- .../src/SplitButton/SplitButton.types.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/split-button/src/SplitButton/SplitButton.types.ts b/packages/split-button/src/SplitButton/SplitButton.types.ts index fe61ced90e..d302189746 100644 --- a/packages/split-button/src/SplitButton/SplitButton.types.ts +++ b/packages/split-button/src/SplitButton/SplitButton.types.ts @@ -1,4 +1,4 @@ -import { MouseEventHandler, ReactElement } from 'react'; +import { MouseEventHandler } from 'react'; import { type ButtonProps as ImportedButtonProps, @@ -6,25 +6,14 @@ import { } from '@leafygreen-ui/button'; import { DarkModeProps } from '@leafygreen-ui/lib'; import { - MenuItemProps, type MenuProps as ImportedMenuProps, } from '@leafygreen-ui/menu'; -import { - InferredPolymorphicProps, - PolymorphicAs, -} from '@leafygreen-ui/polymorphic'; import { Align as ImportedAlign, Justify as ImportedJustify, } from '@leafygreen-ui/popover'; import type { ItemClickHandler } from '../Menu/Menu.types'; -export type MenuItemType = ReactElement< - InferredPolymorphicProps ->; - -export type MenuItemsType = Array; - export const Variant = { Default: ButtonVariants.Default, Primary: ButtonVariants.Primary,