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

Move the call to onDatesChange before the call to onFocusChange #1525

Merged
merged 5 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@nkinser
Copy link
Contributor

nkinser commented Feb 5, 2019

This PR moves the call to onDatesChange before onFocusChange. The purpose of this change is to allow the logic of onDatesChange to potentially affect the logic of onFocusChange.

For example, I would like to allow users to click on certain unavailable dates and then show them a corresponding error state. When such a date is clicked, I do not want the focus to change because the date they clicked would not actually be selected. The logic of whether or not to change focus needs to occur in onDatesChange, so this function needs to be called before onFocusChange.

@nkinser

This comment has been minimized.

Copy link
Contributor Author

nkinser commented Feb 5, 2019

@majapw PTAL!
This passes all the tests and I've also checked in storybook to make sure everything is acting as expected. Is there anything I should do to test this more extensively?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 5, 2019

If the ordering is important, then I think we should add a test that asserts it (also, you'll need to rebase)

@nkinser nkinser force-pushed the nkinser:nk--couple-dates-change-with-focus-change branch from da543b2 to 957c028 Feb 5, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 5, 2019

Coverage Status

Coverage decreased (-0.005%) to 84.979% when pulling 95ee636 on nkinser:nk--couple-dates-change-with-focus-change into f596ba2 on airbnb:master.

@majapw

majapw approved these changes Feb 5, 2019

Copy link
Contributor

majapw left a comment

@ljharb would you say this is a breaking change? I'm worried about some edge-case of behavior being dependent on this.

@ljharb
Copy link
Member

ljharb left a comment

I think I might feel better if we could add the tests to master, and then change them with this PR - that would clearly illustrate what was breaking or not.

@ljharb
Copy link
Member

ljharb left a comment

Thanks, the added tests look great

Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated
Show resolved Hide resolved test/components/DayPickerRangeController_spec.jsx Outdated

@nkinser nkinser force-pushed the nkinser:nk--couple-dates-change-with-focus-change branch from 50e2e7d to 95ee636 Feb 5, 2019

@nkinser

This comment has been minimized.

Copy link
Contributor Author

nkinser commented Feb 6, 2019

@majapw @ljharb Do you know when we will be able to release this?

@ljharb

ljharb approved these changes Feb 6, 2019

wrapper.instance().onDayClick(clickDate);
expect(focusedInput).to.equal(END_DATE);

This comment has been minimized.

@ljharb

ljharb Feb 6, 2019

Member

this does seem like a possible breaking change to me. @majapw?

This comment has been minimized.

@majapw

majapw Feb 6, 2019

Contributor

Yep! I'm gonna release it as v19 after I release the previous changes in a minor.

@majapw majapw merged commit a7dbbde into airbnb:master Feb 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.005%) to 84.979%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment