Reset DateInput field value after state change. #5517
Conversation
AlbertoTonegari
changed the title
There was a problem hiding this comment.
Thanks for finding and including a fix. One other thing, can you file an issue in GitHub for this as well? Should take but a moment https://github.com/grommet/grommet/issues/new
| @@ -74,8 +74,12 @@ const DateInput = forwardRef( | |||
| useEffect(() => { | |||
| if ( | |||
| schema && | |||
| value && | |||
| ((Array.isArray(value) && value[0]) || !Array.isArray(value)) | |||
| (value || value?.length === 0) && | |||
There was a problem hiding this comment.
Could this be simplified to value !== undefined?
There was a problem hiding this comment.
Thank you for the review.
I have simplified that expression to the following:
![undefined, null].includes(value) &&
(Array.isArray(value) || !Array.isArray(value))
What do you think @ericsoderberghp ?
| ( | ||
| (Array.isArray(value) && value[0]) | ||
| || !Array.isArray(value) | ||
| || value?.length === 0 |
There was a problem hiding this comment.
I think was checked on line 77 already. I suspect you mean value[0]?.length === 0? In which case, I think line 79 could be: (Array.isArray(value && value[0] !== undefined) || !Array.isArray(value)
There was a problem hiding this comment.
Hello, thank you for the review.
Unfortunately, this will not work because I want to reset the date when I pass an [ ] as a value. And in this case value[0] !== undefined will be false.
| value && | ||
| ((Array.isArray(value) && value[0]) || !Array.isArray(value)) | ||
| ![undefined, null].includes(value) && | ||
| (Array.isArray(value) || !Array.isArray(value)) |
There was a problem hiding this comment.
Won't this logic always return true, since the value is either an Array or not an Array?
I'm thinking undefined !== value might be enough. That would align with how undefined is what determines whether an input component is controlled or not. In other words, I don't think we need to also allow for null.
There was a problem hiding this comment.
You are absolutely right. 😅
I have modified it.
|
Thanks for your contribution! |
What does this PR do?
This PR adds the possibility to reset the DateInput field on state change.
Where should the reviewer start?
From
DateInput.jsWhat testing has been done on this PR?
I have added 2 unit tests for the new functionality.
I added 1 story called ResetDate.js and tested the functionality there.
How should this be manually tested?
After reviewing
DateInput.jsandResetDate.jsthe functionality should be tested by opening theResetDate.jsstory.Any background context you want to provide?
What are the relevant issues?
The issue is that on state change, there is no possibility to clear the
DateInputfield.Here is a codesandbox showcasing the issue that the DateInput field can't be cleared.
https://codesandbox.io/s/recursing-star-3tnfk
Screenshots (if appropriate)
Do the grommet docs need to be updated?
I don't think so.
Should this PR be mentioned in the release notes?
Yes, absolutely.
Is this change backwards compatible or is it a breaking change?
From my knowledge, I think it's backwards compatible.