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

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

Merged

Conversation

nkinser
Copy link
Contributor

@nkinser 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
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
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 nk--couple-dates-change-with-focus-change branch from da543b2 to 957c028 Compare February 5, 2019 00:11
@coveralls
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.

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/components/DayPickerRangeController.jsx Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the added tests look great

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 Show resolved Hide resolved
@nkinser nkinser force-pushed the nk--couple-dates-change-with-focus-change branch from 50e2e7d to 95ee636 Compare February 5, 2019 22:37
@nkinser
Copy link
Contributor Author

nkinser commented Feb 6, 2019

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@majapw majapw Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@majapw majapw merged commit a7dbbde into react-dates:master Feb 6, 2019
@ljharb ljharb added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants