Skip to content
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

Merged
merged 4 commits into from
Oct 18, 2019

Conversation

MitchRivet
Copy link
Contributor

@MitchRivet MitchRivet commented Oct 6, 2018

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:

screen shot 2018-10-05 at 5 13 31 pm

Also, I added a test to cover this scenario.

@codecov
Copy link

codecov bot commented Oct 6, 2018

Codecov Report

Merging #816 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #816   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...89b777d. Read the comment docs.

@andrewsuzuki
Copy link

Looks solid to me, @gpbl can you review?

@jordan-jarolim
Copy link

First of all, thank you for your work! Will this be merged any soon? :)

@MitchRivet
Copy link
Contributor Author

@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

@tanelih
Copy link

tanelih commented Feb 27, 2019

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 typedValue (https://github.com/gpbl/react-day-picker/pull/816/files#diff-e2a925e665d836a09575f79095191746R574)

This leads to a situation where setting the value programmatically to an empty string will still leave the typedValue intact inside the component's state - thus rendering it instead of the "reset" value.

I did some digging around and found out that one could "reset" the typedValue in the componentDidUpdate function (https://github.com/gpbl/react-day-picker/blob/master/src/DayPickerInput.js#L187):

// 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 " ".

@aekt
Copy link

aekt commented Mar 30, 2019

@tanelih I totally agree with you! It seems like no way to reset the input field now... Based on this PR, setting value state to " " can indeed empty the field, but it may not be very satisfying if one wants placeholder to be shown. So, we probably need some way to reset typedValue?

Wouldn't it be a good if we directly expose a method to reset this typedValue state? I think such way will be more flexible and explicit.

@KumailP
Copy link

KumailP commented Apr 6, 2019

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.

@dmitrykrylov
Copy link

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 :(

@jqueryisamonad
Copy link

@gpbl merge please 😢

@mdeeter
Copy link

mdeeter commented Oct 17, 2019

Can this get merged?

@gpbl gpbl added this to the v7.2.0 milestone Oct 18, 2019
@gpbl gpbl merged commit 81ac116 into gpbl:master Oct 18, 2019
@ntlf ntlf mentioned this pull request Feb 18, 2020
kimamula pushed a commit to kimamula/react-day-picker that referenced this pull request Aug 17, 2022
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants