From 64f726fcf6ca90859560af272ac804f144409a5e Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 6 Jun 2024 18:36:31 +0200 Subject: [PATCH 1/5] wip: add mergeAnchorHandlers --- packages/react/src/ActionMenu/ActionMenu.tsx | 27 +++++- .../react/src/__tests__/ActionMenu.test.tsx | 83 +++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 619272c40b1..c8d05dab4cc 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -43,6 +43,31 @@ export type ActionMenuProps = { onOpenChange?: (s: boolean) => void } & Pick +// anchorProps adds onClick and onKeyDown, so we need to merge them with buttonProps +const mergeAnchorHandlers = (anchorProps: React.HTMLAttributes, buttonProps: ButtonProps) => { + const mergedAnchorProps = {...anchorProps} + + if (typeof buttonProps.onClick === 'function') { + const anchorOnClick = anchorProps.onClick + const mergedOnClick = (event: React.MouseEvent) => { + buttonProps.onClick?.(event) + anchorOnClick?.(event) + } + mergedAnchorProps.onClick = mergedOnClick + } + + if (typeof buttonProps.onKeyDown === 'function') { + const anchorOnKeyDown = anchorProps.onKeyDown + const mergedOnAnchorKeyDown = (event: React.KeyboardEvent) => { + buttonProps.onKeyDown?.(event) + anchorOnKeyDown?.(event) + } + mergedAnchorProps.onKeyDown = mergedOnAnchorKeyDown + } + + return mergedAnchorProps +} + const Menu: React.FC> = ({ anchorRef: externalAnchorRef, open, @@ -111,7 +136,7 @@ const Menu: React.FC> = ({ } return null } else if (child.type === MenuButton) { - renderAnchor = anchorProps => React.cloneElement(child, anchorProps) + renderAnchor = anchorProps => React.cloneElement(child, mergeAnchorHandlers(anchorProps, child.props)) return null } else { return child diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index 91533a83b3a..e9a8ae34de9 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -632,4 +632,87 @@ describe('ActionMenu', () => { expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true') }) }) + + describe('event handlers', () => { + it('should call onClick and onKeyDown passed to ActionMenu.Button', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + Open menu + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + + it.only('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + }) }) From 4adff1a3d177066cecc8861ee6d972d031a0598f Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 18 Jun 2024 12:42:56 +0200 Subject: [PATCH 2/5] merged onClick and onKeyDown in ActionMenu.Anchor --- packages/react/src/ActionMenu/ActionMenu.tsx | 21 +++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index c8d05dab4cc..de64f8c93c4 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -162,17 +162,27 @@ const Menu: React.FC> = ({ } export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} -const Anchor = React.forwardRef(({children, ...anchorProps}, anchorRef) => { +const Anchor = React.forwardRef(({children: child, ...anchorProps}, anchorRef) => { const {onOpen, isSubmenu} = React.useContext(MenuContext) const openSubmenuOnRightArrow: React.KeyboardEventHandler = useCallback( event => { - children.props.onKeyDown?.(event) if (isSubmenu && event.key === 'ArrowRight' && !event.defaultPrevented) onOpen?.('anchor-key-press') }, - [children, isSubmenu, onOpen], + [isSubmenu, onOpen], ) + const onButtonClick = (event: React.MouseEvent) => { + child.props.onClick?.(event) + anchorProps?.onClick(event) + } + + const onButtonKeyDown = (event: React.KeyboardEvent) => { + child.props.onKeyDown?.(event) + openSubmenuOnRightArrow(event) + anchorProps?.onKeyDown(event) + } + // Add right chevron icon to submenu anchors rendered using `ActionList.Item` const parentActionListContext = useContext(ActionListContainerContext) const thisActionListContext = useMemo( @@ -190,10 +200,11 @@ const Anchor = React.forwardRef(({children, return ( - {React.cloneElement(children, { + {React.cloneElement(child, { ...anchorProps, ref: anchorRef, - onKeyDown: openSubmenuOnRightArrow, + onClick: onButtonClick, + onKeyDown: onButtonKeyDown, })} ) From ba8139fc356b3310b7aaf52ff8c0c60ada9e4128 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 18 Jun 2024 12:59:53 +0200 Subject: [PATCH 3/5] improve types --- packages/react/src/ActionMenu/ActionMenu.tsx | 6 +++--- packages/react/src/__tests__/ActionMenu.test.tsx | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index de64f8c93c4..1a1388193f1 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -161,7 +161,7 @@ const Menu: React.FC> = ({ ) } -export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} +export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} & React.HTMLAttributes const Anchor = React.forwardRef(({children: child, ...anchorProps}, anchorRef) => { const {onOpen, isSubmenu} = React.useContext(MenuContext) @@ -174,13 +174,13 @@ const Anchor = React.forwardRef(({children: const onButtonClick = (event: React.MouseEvent) => { child.props.onClick?.(event) - anchorProps?.onClick(event) + anchorProps.onClick?.(event) // onClick is passed from AnchoredOverlay } const onButtonKeyDown = (event: React.KeyboardEvent) => { child.props.onKeyDown?.(event) openSubmenuOnRightArrow(event) - anchorProps?.onKeyDown(event) + anchorProps.onKeyDown?.(event) // onKeyDown is passed from AnchoredOverlay } // Add right chevron icon to submenu anchors rendered using `ActionList.Item` diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index e9a8ae34de9..427c2e5f855 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -672,7 +672,7 @@ describe('ActionMenu', () => { expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) - it.only('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => { + it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => { const mockOnClick = jest.fn() const mockOnKeyDown = jest.fn() From 229dd54d35296365706b05d0709504f67b4d0051 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 18 Jun 2024 13:00:59 +0200 Subject: [PATCH 4/5] Create green-schools-smell.md --- .changeset/green-schools-smell.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/green-schools-smell.md diff --git a/.changeset/green-schools-smell.md b/.changeset/green-schools-smell.md new file mode 100644 index 00000000000..aa283a70445 --- /dev/null +++ b/.changeset/green-schools-smell.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +ActionMenu: Make sure event handlers on ActionMenu.Button and ActionMenu.Anchor are called From dbecfcfaeb1ad68bca57bda9ee1c9633a312cde7 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 18 Jun 2024 14:59:24 +0200 Subject: [PATCH 5/5] cover cases with Tooltip --- packages/react/src/ActionMenu/ActionMenu.tsx | 10 ++- .../react/src/__tests__/ActionMenu.test.tsx | 87 ++++++++++++++++++- 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 1a1388193f1..5d1e614d5f9 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -112,7 +112,10 @@ const Menu: React.FC> = ({ if (anchorChildren.type === MenuButton) { renderAnchor = anchorProps => { // We need to attach the anchor props to the tooltip trigger (ActionMenu.Button's grandchild) not the tooltip itself. - const triggerButton = React.cloneElement(anchorChildren, {...anchorProps}) + const triggerButton = React.cloneElement( + anchorChildren, + mergeAnchorHandlers({...anchorProps}, anchorChildren.props), + ) return React.cloneElement(child, {children: triggerButton, ref: anchorRef}) } } @@ -126,7 +129,10 @@ const Menu: React.FC> = ({ // ActionMenu.Anchor's children can be wrapped with Tooltip. If this is the case, our anchor is the tooltip's trigger const tooltipTrigger = anchorChildren.props.children // We need to attach the anchor props to the tooltip trigger not the tooltip itself. - const tooltipTriggerEl = React.cloneElement(tooltipTrigger, {...anchorProps}) + const tooltipTriggerEl = React.cloneElement( + tooltipTrigger, + mergeAnchorHandlers({...anchorProps}, tooltipTrigger.props), + ) const tooltip = React.cloneElement(anchorChildren, {children: tooltipTriggerEl}) return React.cloneElement(child, {children: tooltip, ref: anchorRef}) } diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index 427c2e5f855..5ca327e4c35 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -633,7 +633,7 @@ describe('ActionMenu', () => { }) }) - describe('event handlers', () => { + describe('calls event handlers on trigger', () => { it('should call onClick and onKeyDown passed to ActionMenu.Button', async () => { const mockOnClick = jest.fn() const mockOnKeyDown = jest.fn() @@ -714,5 +714,90 @@ describe('ActionMenu', () => { expect(component.queryByRole('menu')).toBeInTheDocument() expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) + + it('should call onClick and onKeyDown passed to ActionMenu.Button with Tooltip', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + Open menu + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + + it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor with Tooltip', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) }) })