diff --git a/.changeset/modern-files-suffer.md b/.changeset/modern-files-suffer.md new file mode 100644 index 00000000000..f09c7ad47e2 --- /dev/null +++ b/.changeset/modern-files-suffer.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +experimental/SelectPanel: Fix bug where onSubmit also called onCancel! diff --git a/src/drafts/SelectPanel2/SelectPanel.test.tsx b/src/drafts/SelectPanel2/SelectPanel.test.tsx new file mode 100644 index 00000000000..fc7dcb5e445 --- /dev/null +++ b/src/drafts/SelectPanel2/SelectPanel.test.tsx @@ -0,0 +1,132 @@ +import React from 'react' +import {ThemeProvider, ActionList} from '../../' +import {render, RenderResult} from '@testing-library/react' +import userEvent, {UserEvent} from '@testing-library/user-event' +import data from './stories/mock-data' +import {SelectPanel, SelectPanelProps} from './SelectPanel' + +const Fixture = ({onSubmit, onCancel}: Pick) => { + const initialSelectedLabels = data.issue.labelIds // mock initial state: has selected labels + const [selectedLabelIds, setSelectedLabelIds] = React.useState(initialSelectedLabels) + + /* Selection */ + const onLabelSelect = (labelId: string) => { + if (!selectedLabelIds.includes(labelId)) setSelectedLabelIds([...selectedLabelIds, labelId]) + else setSelectedLabelIds(selectedLabelIds.filter(id => id !== labelId)) + } + + const itemsToShow = data.labels + + return ( + + + Assign label + + + {itemsToShow.map(label => ( + onLabelSelect(label.id)} + selected={selectedLabelIds.includes(label.id)} + > + {label.name} + {label.description} + + ))} + + + + + ) +} + +describe('SelectPanel', () => { + it('renders Button by default', async () => { + const container = render() + + const trigger = container.getByRole('button') + expect(trigger).toBeInTheDocument() + expect(container.queryByRole('dialog')).toBeNull() + }) + + it('opens Dialog on Button click', async () => { + const container = render() + const user = userEvent.setup() + + expect(container.queryByRole('dialog')).toBeNull() + + const trigger = container.getByRole('button') + await user.click(trigger) + + expect(container.queryByRole('dialog')).toBeInTheDocument() + }) + + type MockFunctions = {mockOnSubmit?: () => void; mockOnCancel?: () => void} + const getFixtureWithOpenContainer = async ({mockOnSubmit, mockOnCancel}: MockFunctions = {}) => { + const container = render() + const user = await userEvent.setup() + + const trigger = container.getByRole('button') + await user.click(trigger) + + return {container, user} + } + + it('clicking an item selects it', async () => { + const {container, user} = await getFixtureWithOpenContainer() + + const initiallyUnselectedOption = container.getAllByRole('option')[2] + expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'false') + + await user.click(initiallyUnselectedOption) + expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'true') + }) + + const selectUnselectedOption = async (container: RenderResult, user: UserEvent) => { + const initiallyUnselectedOption = container.getAllByRole('option')[2] + expect(initiallyUnselectedOption).toHaveAttribute('aria-selected', 'false') + await user.click(initiallyUnselectedOption) + } + + it('submit closes the dialog and calls onSubmit', async () => { + const mockOnSubmit = jest.fn() + const mockOnCancel = jest.fn() + const {container, user} = await getFixtureWithOpenContainer({mockOnSubmit, mockOnCancel}) + selectUnselectedOption(container, user) + + const submitButton = container.getByRole('button', {name: 'Save'}) + await user.click(submitButton) + + expect(container.queryByRole('dialog')).toBeNull() + expect(mockOnSubmit).toHaveBeenCalledTimes(1) + expect(mockOnCancel).toHaveBeenCalledTimes(0) + }) + + it('cancel 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) + + const cancelButton = container.getByRole('button', {name: 'Cancel'}) + await user.click(cancelButton) + + expect(container.queryByRole('dialog')).toBeNull() + expect(mockOnCancel).toHaveBeenCalledTimes(1) + expect(mockOnSubmit).toHaveBeenCalledTimes(0) + }) + + it('close button 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) + + const closeButton = container.getByRole('button', {name: 'Close'}) + await user.click(closeButton) + + expect(container.queryByRole('dialog')).toBeNull() + expect(mockOnCancel).toHaveBeenCalledTimes(1) + expect(mockOnSubmit).toHaveBeenCalledTimes(0) + }) +}) diff --git a/src/drafts/SelectPanel2/SelectPanel.tsx b/src/drafts/SelectPanel2/SelectPanel.tsx index 59fe6164ca6..b77c866e8bd 100644 --- a/src/drafts/SelectPanel2/SelectPanel.tsx +++ b/src/drafts/SelectPanel2/SelectPanel.tsx @@ -98,7 +98,7 @@ const Panel: React.FC = ({ const onAnchorClick = () => { if (!internalOpen) setInternalOpen(true) - else onInternalClose() + else onInternalCancel() } const contents = React.Children.map(props.children, child => { @@ -115,14 +115,19 @@ const Panel: React.FC = ({ return child }) - const onInternalClose = () => { + const onInternalClose = React.useCallback(() => { + if (internalOpen === false) return // nothing to do here if (propsOpen === undefined) setInternalOpen(false) + }, [internalOpen, propsOpen]) + + const onInternalCancel = () => { + onInternalClose() if (typeof propsOnCancel === 'function') propsOnCancel() } const onInternalSubmit = (event?: React.FormEvent) => { event?.preventDefault() // there is no event with selectionVariant=instant - if (propsOpen === undefined) setInternalOpen(false) + onInternalClose() if (typeof propsOnSubmit === 'function') propsOnSubmit(event) } @@ -152,17 +157,32 @@ const Panel: React.FC = ({ /* Dialog */ const dialogRef = React.useRef(null) - if (internalOpen) dialogRef.current?.showModal() - else dialogRef.current?.close() + + // sync dialog open state (imperative) with internal component state + React.useEffect(() => { + if (internalOpen) dialogRef.current?.showModal() + else if (dialogRef.current?.open) dialogRef.current.close() + }, [internalOpen]) // dialog handles Esc automatically, so we have to sync internal state + // but it doesn't call onCancel, so have another effect for that! + React.useEffect(() => { + const dialogEl = dialogRef.current + dialogEl?.addEventListener('close', onInternalClose) + return () => dialogEl?.removeEventListener('close', onInternalClose) + }, [onInternalClose]) + + // Esc handler React.useEffect(() => { - const dialog = dialogRef.current - dialog?.addEventListener('close', onInternalClose) - return () => dialog?.removeEventListener('close', onInternalClose) + const dialogEl = dialogRef.current + const handler = (event: KeyboardEvent) => { + if (event.key === 'Escape') onInternalCancel() + } + dialogEl?.addEventListener('keydown', handler) + return () => dialogEl?.removeEventListener('keydown', handler) }) - // React doesn't support autoFocus for dialog: https://github.com/facebook/react/issues/23301 + // 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, // but not for dialogs, so we have to do it React.useEffect(() => { @@ -232,7 +252,7 @@ const Panel: React.FC = ({ panelId, title, description, - onCancel: onInternalClose, + onCancel: onInternalCancel, onClearSelection: propsOnClearSelection ? onInternalClearSelection : undefined, searchQuery, setSearchQuery, diff --git a/src/utils/test-helpers.tsx b/src/utils/test-helpers.tsx index bcca7a6d1cc..05b286d06b0 100644 --- a/src/utils/test-helpers.tsx +++ b/src/utils/test-helpers.tsx @@ -18,3 +18,22 @@ global.CSS = { } global.TextEncoder = TextEncoder + +/** + * Required for internal usage of dialog in primer/react + * this is not implemented in JSDOM, and until it is we'll need to polyfill + * https://github.com/jsdom/jsdom/issues/3294 + * https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom + * bonus: we only want to mock browser globals in DOM (or js-dom) environments – not in SSR / node + */ +if (typeof document !== 'undefined') { + global.HTMLDialogElement.prototype.showModal = jest.fn(function mock(this: HTMLDialogElement) { + // eslint-disable-next-line no-invalid-this + this.open = true + }) + + global.HTMLDialogElement.prototype.close = jest.fn(function mock(this: HTMLDialogElement) { + // eslint-disable-next-line no-invalid-this + this.open = false + }) +}