-
-
Notifications
You must be signed in to change notification settings - Fork 734
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
When DayPicker input state is controlled, correctly set the input text on 'value' prop updates #816
Conversation
Codecov Report
@@ Coverage Diff @@
## master #816 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 15 15
Lines 645 645
Branches 141 141
=====================================
Hits 645 645
Continue to review full report at Codecov.
|
Looks solid to me, @gpbl can you review? |
First of all, thank you for your work! Will this be merged any soon? :) |
@jordan-jarolim thx, hopefully soon. haven't heard from any of the maintainers. going to reach out in the gitter and see if there planning on updating/releasing anytime soon |
Related to #804 - with this PR how would one go about setting an empty value for the input (e.g. when resetting the form state). Since empty values (empty strings) are evaluated as "falsy", the value passed to the actual input element will always end up being This leads to a situation where setting the value programmatically to an empty string will still leave the I did some digging around and found out that one could "reset" the // Most likely a very bad solution, but it should convey the idea.
componentDidUpdate (prevProps) {
// ...
if (value !== prevProps.value) {
if (this.props.value === "") {
newState.typedValue = ""
}
// ...
}
// ...
} I'm not sure whether this is something to consider. I might also be using this component wrong. 😄 Luckily this is very easy to work around with a single space |
@tanelih I totally agree with you! It seems like no way to reset the input field now... Based on this PR, setting Wouldn't it be a good if we directly expose a method to reset this |
Isn't this a somewhat core functionality, to be able to reflect the state in the input on update? Either way, great work. Really hope this gets merged soon. |
Hey! Is somebody going to merge this PR? DayPickerInput can't provide good UX now because it allows an invalid date string to be kept as an input value and there is no even way to control it :( |
@gpbl merge please 😢 |
Can this get merged? |
…t on 'value' prop updates (gpbl#816) * fix for to clear input value with controlled value * fix uncontrolled input warning by initializing typedValue and not setting to undefined * add test to cover controlled input value input text update * run linter (oops)
Reported as an issue with explanation here: #815
Essentially, my issue is that when DayPickerInput is controlled (i.e. takes a 'value' prop) and an invalid date is entered into the input box, an update to the value prop does not clear/update the text within the input (even though the overlay correctly updates the state)
The main issue in this case happens here: https://github.com/gpbl/react-day-picker/blob/master/src/DayPickerInput.js#L576
In this case, this.state.typedValue is defined and remains inside the input, even though this.state.value is the correct state of the input.
The other changes address warnings that are shown in the console as a result. This message happens when you initialize a controlled input with an undefined value:
Also, I added a test to cover this scenario.