From 53c6cec7be03662719e2eae5b730ac1fdce294d8 Mon Sep 17 00:00:00 2001 From: Ben Wittenberg Date: Thu, 16 May 2024 16:55:29 +0000 Subject: [PATCH 1/2] fix(SelectPanel2): only bind keydown event when necessary --- .../drafts/SelectPanel2/SelectPanel.test.tsx | 32 +++++++++++++++++++ .../src/drafts/SelectPanel2/SelectPanel.tsx | 6 ++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx b/packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx index bd2127ae8c2..5e54e4c6b3b 100644 --- a/packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx +++ b/packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx @@ -179,6 +179,38 @@ describe('SelectPanel', () => { expect(mockOnSubmit).toHaveBeenCalledTimes(0) }) + it('should not call addEventListener on each render for Escape key handling when onCancel has not changed', async () => { + const onCancel = jest.fn() + const container = render( + + child + , + ) + const addEventListenerSpy = jest.spyOn(globalThis.EventTarget.prototype, 'addEventListener') + const removeEventListenerSpy = jest.spyOn(globalThis.EventTarget.prototype, 'removeEventListener') + + container.rerender( + + child + , + ) + expect(addEventListenerSpy).not.toHaveBeenCalled() + expect(removeEventListenerSpy).not.toHaveBeenCalled() + }) + + it('Escape key closes the dialog and calls onCancel', async () => { + const mockOnSubmit = jest.fn() + const mockOnCancel = jest.fn() + const {container, user} = await getFixtureWithOpenContainer({mockOnSubmit, mockOnCancel}) + selectUnselectedOption(container, user) + + await user.keyboard('{Escape}') + + expect(container.queryByRole('dialog')).toBeNull() + expect(mockOnCancel).toHaveBeenCalledTimes(1) + expect(mockOnSubmit).toHaveBeenCalledTimes(0) + }) + it('SelectPanel within FormControl should be labelled by FormControl.Label', async () => { const component = render() const buttonByRole = component.getByRole('button') diff --git a/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx b/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx index c882202602c..ab99dbbe433 100644 --- a/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx @@ -136,10 +136,10 @@ const Panel: React.FC = ({ if (propsOpen === undefined) setInternalOpen(false) }, [internalOpen, propsOpen]) - const onInternalCancel = () => { + const onInternalCancel = React.useCallback(() => { onInternalClose() if (typeof propsOnCancel === 'function') propsOnCancel() - } + }, [onInternalClose, propsOnCancel]) const onInternalSubmit = (event?: React.FormEvent) => { event?.preventDefault() // there is no event with selectionVariant=instant @@ -199,7 +199,7 @@ const Panel: React.FC = ({ } dialogEl?.addEventListener('keydown', handler) return () => dialogEl?.removeEventListener('keydown', handler) - }) + }, [onInternalCancel]) // Autofocus hack: React doesn't support autoFocus for dialog: https://github.com/facebook/react/issues/23301 // tl;dr: react takes over autofocus instead of letting the browser handle it, From f332218ed8bc772b4e15d53959a2c66df7fb7e46 Mon Sep 17 00:00:00 2001 From: Ben Wittenberg Date: Thu, 16 May 2024 17:23:23 +0000 Subject: [PATCH 2/2] chore(SelectPanel2): Add changeset --- .changeset/good-bugs-grab.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/good-bugs-grab.md diff --git a/.changeset/good-bugs-grab.md b/.changeset/good-bugs-grab.md new file mode 100644 index 00000000000..39a88eb9094 --- /dev/null +++ b/.changeset/good-bugs-grab.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +SelectPanel2: Minor optimization for escape key event listener binding