-
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
size-limit report 📦
|
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 comment
The reason will be displayed to describe this comment to others. Learn more.
don't worry, this line is part of onInternalClose
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 comment
The 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 :)
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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Done :)
import React from 'react' | ||
import {ThemeProvider, ActionList} from '../../' | ||
import {render, RenderResult} from '@testing-library/react' | ||
import userEvent, {UserEvent} from '@testing-library/user-event' |
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!
Bug: Calling
onSubmit
callsinternalClose
which callsonCancel
🐞!This PR fixes that by separating
internalClose
andinternalCancel