Skip to content

Commit

Permalink
fix(SelectPanel2): only bind keydown event when necessary (#4601)
Browse files Browse the repository at this point in the history
* fix(SelectPanel2): only bind keydown event when necessary

* chore(SelectPanel2): Add changeset
  • Loading branch information
bwittenberg committed May 22, 2024
1 parent 9b63299 commit f57dd3d
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/good-bugs-grab.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

SelectPanel2: Minor optimization for escape key event listener binding
32 changes: 32 additions & 0 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<SelectPanel title="title" onCancel={onCancel}>
child
</SelectPanel>,
)
const addEventListenerSpy = jest.spyOn(globalThis.EventTarget.prototype, 'addEventListener')
const removeEventListenerSpy = jest.spyOn(globalThis.EventTarget.prototype, 'removeEventListener')

container.rerender(
<SelectPanel title="title" onCancel={onCancel}>
child
</SelectPanel>,
)
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(<SelectPanelWithinForm />)
const buttonByRole = component.getByRole('button')
Expand Down
6 changes: 3 additions & 3 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,10 @@ const Panel: React.FC<SelectPanelProps> = ({
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<HTMLFormElement>) => {
event?.preventDefault() // there is no event with selectionVariant=instant
Expand Down Expand Up @@ -199,7 +199,7 @@ const Panel: React.FC<SelectPanelProps> = ({
}
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,
Expand Down

0 comments on commit f57dd3d

Please sign in to comment.