-
Notifications
You must be signed in to change notification settings - Fork 1k
Reset DateInput textValue on form reset #6834
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
Conversation
Converted to draft -- this is a little too generic where any reset event is clearing the field (should be isolated to the form the input is in) |
This is ready for review again, I've added a check to make sure the DateInput is part of the form that was reset. |
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.
Looks good! Thanks for adding a test
window.addEventListener('reset', handleFormReset); | ||
return () => window.removeEventListener('reset', handleFormReset); |
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 a bit of a corner case, but because this expects the event to bubble up, if the Form's onReset stops the event propagation this won't get called. For example I broke this with:
<Form onReset={(e) => {
console.log('MyReset');
e.stopPropagation();
}}>
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.
Hmm -- that's a good note, even if a corner case. I was trying to just rely on native HTML events, but I suppose one option would be to fire a custom event within form. and listen for that within DateInput?
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.
I adjusted the scope of the event listener to be placed on the form as opposed to the window. I checked with the code you shared (with custom reset function) and it worked.
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.
Coolness. Thanks!
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.
Nice work!
What does this PR do?
The MaskedInput inside of DateInput is controlled. Therefore, in an uncontrolled form, when the form is reset, the text value in the DateInput isn't. This PR updates DateInput to listen for the
reset
event to reset the textValue.Where should the reviewer start?
src/js/components/DateInput/DateInput.js
What testing has been done on this PR?
Tested with code snippet provided in #6818
Also tested with:
Added jest test.
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #6818
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No.
Should this PR be mentioned in the release notes?
Yes. Fixed bug where DateInput text value did not reset with Form reset.
Is this change backwards compatible or is it a breaking change?
Backwards compatible.