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

Value is not updated on DayPickerInput when value prop is in a different month #535

Closed
supersime opened this Issue Oct 31, 2017 · 4 comments

Comments

3 participants
@supersime

supersime commented Oct 31, 2017

There is a From and To DayPickerInput element on a page. Both have a value that comes from state.
When the From field is changed, an onDayChange function is used to update the value of the From and To dates in state.
However...
if the new From date is in a different month, then on the very first change of the From date the To date will not be updated.
There appears to be an issue in the DayPickerInput.js, componentWillReceiveProps function whereby this condition (if (hasDifferentValue && !shouldDisplayAnotherMonth)) is causing an issue. Based on my test scenario, hasDifferentValue is true, and shouldDisplayAnotherMonth is also true, and therefore there is no mapping of the nextProps to state - i.e. the state is not updated with the changed date.

Issue has been reproduced at:
https://codesandbox.io/s/yjo1nzx51z

Use the following steps to reproduce:
Scenario 1

  1. From and To dates default to 30-Oct-2017
  2. Change the From date to 15-Nov-17. Note the To date does not change. Expectation is this would have been updated to 15-Nov-17.
  3. Change the From date to 16-Nov-17. Note the To date does change. As expected.

Scenario 2 (NOTE: refresh the browser to start again):

  1. From and To dates default to 30-Oct-2017
  2. Change the From date to 31-Oct-2017. Note the To date does change. As expected.

Scenario 3 (NOTE: refresh the browser to start again):

  1. From and To dates default to 30-Oct-2017
  2. Click on the To date to note that the calendar is displayed in October.
  3. Change the From date to 15-Sep-2017. Note the To date does not change. Expectation is this would have been updated to 15-Sep-2017.
  4. Click on the To date to note that the calendar is displayed in September.
  5. Change the From date to 16-Sep-2017. Note the To date does change. As expected.

@gpbl gpbl added this to the v6.3.0 milestone Oct 31, 2017

@JonHolt

This comment has been minimized.

Show comment
Hide comment
@JonHolt

JonHolt Nov 2, 2017

So I've forked the project planning on making a PR to fix this, but I've run into a problem. I wrote the following unit test expecting it to fail, but it passes.

it('should set the month even if the value is in a different month', () => {
      const wrapper = mount(
        <DayPickerInput value="11/02/2017" format="MM/DD/YYYY" />
      );
      wrapper.setProps({ value: '12/25/2017' });
      expect(wrapper.instance().state.month.toDateString()).toBe(
        new Date('12/25/2017').toDateString()
      );
    });

My next thought was to check how the datePicker handles having the prop passed down to it, but the datePicker isn't even rendered at the point this bug occurs so it shouldn't affect it right? Can you see at a glance what I'm doing wrong here?

JonHolt commented Nov 2, 2017

So I've forked the project planning on making a PR to fix this, but I've run into a problem. I wrote the following unit test expecting it to fail, but it passes.

it('should set the month even if the value is in a different month', () => {
      const wrapper = mount(
        <DayPickerInput value="11/02/2017" format="MM/DD/YYYY" />
      );
      wrapper.setProps({ value: '12/25/2017' });
      expect(wrapper.instance().state.month.toDateString()).toBe(
        new Date('12/25/2017').toDateString()
      );
    });

My next thought was to check how the datePicker handles having the prop passed down to it, but the datePicker isn't even rendered at the point this bug occurs so it shouldn't affect it right? Can you see at a glance what I'm doing wrong here?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 2, 2017

Owner

@JonHolt thanks for looking into the issue! Can’t tell precisely whats going on right now, i’ll look into it better during the weekend.

Anyway - try to wrapper.update() after setting the prop. Enzyme 3 gave me some headache for this.

Owner

gpbl commented Nov 2, 2017

@JonHolt thanks for looking into the issue! Can’t tell precisely whats going on right now, i’ll look into it better during the weekend.

Anyway - try to wrapper.update() after setting the prop. Enzyme 3 gave me some headache for this.

@JonHolt

This comment has been minimized.

Show comment
Hide comment
@JonHolt

JonHolt Nov 3, 2017

I'll give that a shot thanks.

JonHolt commented Nov 3, 2017

I'll give that a shot thanks.

@JonHolt

This comment has been minimized.

Show comment
Hide comment
@JonHolt

JonHolt Nov 6, 2017

Yeah I tried it with the wrapper.update() and it's still passing.

Just a quick update: After playing around with this a bit I've noticed the test is passing because the value of month is actually being updated. Internally the DayPickerInput has the right value, it's just not updating the text. I'll look into this further later.

JonHolt commented Nov 6, 2017

Yeah I tried it with the wrapper.update() and it's still passing.

Just a quick update: After playing around with this a bit I've noticed the test is passing because the value of month is actually being updated. Internally the DayPickerInput has the right value, it's just not updating the text. I'll look into this further later.

@gpbl gpbl closed this in #562 Nov 24, 2017

@gpbl gpbl modified the milestones: v6.3.0, v7.0.0 Nov 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment