-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SelectPanel2: Fix bug calling onCancel #4131
Changes from 7 commits
d4006d2
83582e5
977f266
b2c3cfb
e0b151f
9664488
afe464b
e839db2
496ac18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@primer/react": patch | ||
--- | ||
|
||
experimental/SelectPanel: Fix bug where onSubmit also called onCancel! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<SelectPanelProps, 'onSubmit' | 'onCancel'>) => { | ||
const initialSelectedLabels = data.issue.labelIds // mock initial state: has selected labels | ||
const [selectedLabelIds, setSelectedLabelIds] = React.useState<string[]>(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 ( | ||
<ThemeProvider> | ||
<SelectPanel title="Select labels" onSubmit={onSubmit} onCancel={onCancel}> | ||
<SelectPanel.Button>Assign label</SelectPanel.Button> | ||
|
||
<ActionList> | ||
{itemsToShow.map(label => ( | ||
<ActionList.Item | ||
key={label.id} | ||
onSelect={() => onLabelSelect(label.id)} | ||
selected={selectedLabelIds.includes(label.id)} | ||
> | ||
{label.name} | ||
<ActionList.Description variant="block">{label.description}</ActionList.Description> | ||
</ActionList.Item> | ||
))} | ||
</ActionList> | ||
<SelectPanel.Footer /> | ||
</SelectPanel> | ||
</ThemeProvider> | ||
) | ||
} | ||
|
||
describe('SelectPanel', () => { | ||
it('renders Button by default', async () => { | ||
const container = render(<Fixture />) | ||
|
||
const trigger = container.getByRole('button') | ||
expect(trigger).toBeInTheDocument() | ||
expect(container.queryByRole('dialog')).toBeNull() | ||
}) | ||
|
||
it('opens Dialog on Button click', async () => { | ||
const container = render(<Fixture />) | ||
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(<Fixture onSubmit={mockOnSubmit} onCancel={mockOnCancel} />) | ||
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) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,7 +96,7 @@ const Panel: React.FC<SelectPanelProps> = ({ | |
|
||
const onAnchorClick = () => { | ||
if (!internalOpen) setInternalOpen(true) | ||
else onInternalClose() | ||
else onInternalCancel() | ||
} | ||
|
||
const contents = React.Children.map(props.children, child => { | ||
|
@@ -114,13 +114,18 @@ const Panel: React.FC<SelectPanelProps> = ({ | |
}) | ||
|
||
const onInternalClose = () => { | ||
if (internalOpen === false) return // nothing to do here | ||
if (propsOpen === undefined) setInternalOpen(false) | ||
} | ||
|
||
const onInternalCancel = () => { | ||
onInternalClose() | ||
if (typeof propsOnCancel === 'function') propsOnCancel() | ||
} | ||
|
||
const onInternalSubmit = (event?: React.FormEvent<HTMLFormElement>) => { | ||
event?.preventDefault() // there is no event with selectionVariant=instant | ||
if (propsOpen === undefined) setInternalOpen(false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't worry, this line is part of |
||
onInternalClose() | ||
if (typeof propsOnSubmit === 'function') propsOnSubmit(event) | ||
} | ||
|
||
|
@@ -150,13 +155,32 @@ const Panel: React.FC<SelectPanelProps> = ({ | |
|
||
/* Dialog */ | ||
const dialogRef = React.useRef<HTMLDialogElement>(null) | ||
if (internalOpen) dialogRef.current?.showModal() | ||
else dialogRef.current?.close() | ||
|
||
// sync dialog open state (imperative) with internal component state | ||
React.useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved these statements into an effect to avoid conflict with the subsequent lines in the component in the same render dialog was closing and then the other lines in the component would try to access dialogRef.current which throw an error on the console :) |
||
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 | ||
React.useEffect(() => dialogRef.current?.addEventListener('close', onInternalClose)) | ||
// 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) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need these effects to run every render? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! I'll double check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: Done :) |
||
|
||
// Esc handler | ||
React.useEffect(() => { | ||
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(() => { | ||
|
@@ -226,7 +250,7 @@ const Panel: React.FC<SelectPanelProps> = ({ | |
panelId, | ||
title, | ||
description, | ||
onCancel: onInternalClose, | ||
onCancel: onInternalCancel, | ||
onClearSelection: propsOnClearSelection ? onInternalClearSelection : undefined, | ||
searchQuery, | ||
setSearchQuery, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oo UserEvent type, I've never used it before 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really nice!