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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add date offset functionality to DateRangePicker #1252

Merged

Conversation

jakeclements
Copy link
Contributor

As requested in #1062, this PR passes date offset functionality up to the DateRangePicker.

Tests for date offsets are already included within the DayPickerRangeController spec.
If tests are required for the additional functionality this adds to the DateRangePicker please point me to the spec and I'll add them, an appropriate file to add these wasn't clear to me. 馃槄

I'm happy to discuss the inclusion of this PR.

@coveralls
Copy link

coveralls commented Jul 9, 2018

Coverage Status

Coverage increased (+0.02%) to 84.984% when pulling 2355698 on jakeclements:date-offset-in-DateRangePicker into 3b23397 on airbnb:master.

@danilowoz
Copy link

Merge please 馃檹

ljharb
ljharb previously requested changes Aug 23, 2018
test/components/DayPickerRangeController_spec.jsx Outdated Show resolved Hide resolved
test/components/DayPickerRangeController_spec.jsx Outdated Show resolved Hide resolved
src/components/DateRangePicker.jsx Show resolved Hide resolved
@ljharb ljharb added semver-minor: new stuff Any feature or API addition. needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. labels Aug 23, 2018
@jakeclements jakeclements force-pushed the date-offset-in-DateRangePicker branch 3 times, most recently from bb42bfa to ca84fb0 Compare September 11, 2018 03:17
@jakeclements
Copy link
Contributor Author

Thanks for the review @ljharb, I've updated this PR to align with master and actioned your requests. If you see anything else you think needs an update please let me know.

@ljharb ljharb dismissed their stale review September 11, 2018 04:15

LGTM, deferring to @majapw

@ljharb ljharb requested a review from majapw September 11, 2018 04:15
@ljharb ljharb removed the needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. label Sep 11, 2018
@mcfaith9
Copy link

Is this still up? i have same problem/request #1062

@jakeclements
Copy link
Contributor Author

Hey @majapw, there seems to be some interest in this. If you're able to review the changes I'm happy to continue supporting this to get it merged.

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.

Seems good to me. I have one quick q about that added code to the DayPickerRangeController

@@ -483,6 +483,10 @@ export default class DayPickerRangeController extends BaseClass {
startDate = getSelectedDateOffset(startDateOffset, day);
endDate = getSelectedDateOffset(endDateOffset, day);

if (this.isBlocked(startDate) || this.isBlocked(endDate)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this behavior broken before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, I noticed it in making these updates.

@majapw
Copy link
Collaborator

majapw commented Feb 1, 2019

Happy to merge it after the conflict is resolved @jakeclements

Man, this really dropped off my radar. Super sorry! :) Would have merged earlier if I had realized it was such a quick change. 馃檴

@jakeclements
Copy link
Contributor Author

No worries @majapw, I appreciate the quality of this component and the work that you and @ljharb have put into maintaining it!

I've rebased this PR to include the latest changes and resolve that conflict. 馃帀

@majapw majapw merged commit f4e8e0a into react-dates:master Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor: new stuff Any feature or API addition.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants