-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix Save button in password change form #5256
Conversation
🦋 Changeset detectedLatest commit: b507160 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fireEvent.click(submitButton); | ||
|
||
// Assert | ||
expect(defaultProps.onSubmit).toHaveBeenCalled(); |
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.
suggestion: You could also check arguments passed to onsubmit function
it("renders the form", () => { | ||
// Arrange & Act | ||
render(<StaffPasswordResetDialog {...defaultProps} />); | ||
|
||
// Assert | ||
expect(screen.getByRole("form")).toBeInTheDocument(); | ||
}); |
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.
is there any sense testing just the rendering part? you don't test any interaction here, as if it was static content
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.
The bug here was that the "form" wasn't rendered at all (Modal rendered it as div)
/> | ||
<Form initial={initialForm} onSubmit={onSubmit} role="form"> | ||
{({ change, data }) => ( | ||
<Box display="flex" flexDirection="column" gap={4}> |
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.
Suggestion: if you see that big components and you touch them significantly, try next time break them down into smaller ones (context-wise), so perhaps you can test them
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.
Noted, although here there were basically 2 inputs and a button 😄 I know git shows a big change but I only moved the form inside modal content.
* fix Save button in password change form * fix type * change header placement * expect onSubmit to be called with data --------- Co-authored-by: Paweł Chyła <chyla1988@gmail.com>
It fixes an issue that prevented
<form />
from rendering.What type of PR is this?
Related Issues or Documents
Usage Instructions, Screenshots, Recordings
Have you written tests?
[Optional] Description