[Issue 349] Add single-click range picker #884

jakeclements · 2017-12-06T00:06:15Z

This adds the feature requested in #349 by allowing a range prop to be added to the DayPickerRangeController.
The range prop requires an object with optional before / after keys, these expect a function that is passed a moment object of the hovered day.
Return a moment object from each function to specify the range.

Eg. Selecting 5 days (hovered day is not inclusive)

range = {
  before: day => day.subtract(2, 'days'),
  after: day => day.add(2, 'days')
}

I've added storybook examples to show how this works across a couple of scenarios.
I'm happy to hear feedback on this and make changes to ensure it fits within your project

coveralls · 2017-12-06T00:12:17Z

Coverage increased (+0.05%) to 86.76% when pulling 9dc7e04 on jakeclements:custom-range-picker into e92deb3 on airbnb:master.

ljharb

Thanks for the contribution!

ljharb · 2017-12-06T00:08:04Z

examples/DayPickerRangeControllerWrapper.jsx

@@ -18,6 +18,10 @@ const propTypes = forbidExtraProps({
  autoFocusEndDate: PropTypes.bool,
  initialStartDate: momentPropTypes.momentObj,
  initialEndDate: momentPropTypes.momentObj,
+  range: PropTypes.shape({
+    before: PropTypes.func,


This propType also allows an empty object; it seems like either one or both of these properties is required? If so, perhaps something like or([justBefore, justAfter, bothBeforeAndAfter]) might be better?

That's right, at least one is required, I can make an update here.

ljharb · 2017-12-06T00:08:45Z

src/components/DayPickerRangeController.jsx

@@ -39,6 +39,10 @@ const propTypes = forbidExtraProps({
  startDate: momentPropTypes.momentObj,
  endDate: momentPropTypes.momentObj,
  onDatesChange: PropTypes.func,
+  range: PropTypes.shape({


please store this propType somewhere reusable, so it can be shared wherever it's needed.

ljharb · 2017-12-06T00:09:01Z

src/components/DayPickerRangeController.jsx

@@ -413,36 +418,49 @@ export default class DayPickerRangeController extends React.Component {
  }

  onDayClick(day, e) {
-    const { keepOpenOnDateSelect, minimumNights, onBlur } = this.props;
+    const {
+      keepOpenOnDateSelect, minimumNights, onBlur, range,


if the entire statement can't fit on one line, each property needs to be on its own line.

ljharb · 2017-12-06T00:09:18Z

src/components/DayPickerRangeController.jsx

-    if (focusedInput === START_DATE) {
-      onFocusChange(END_DATE);
+    if (range) {
+      startDate = range.before ? range.before(day.clone()) : day.clone();


there's 4 clones here; are all 4 needed?

They're not, I'll clean this up.

ljharb · 2017-12-06T00:09:37Z

src/components/DayPickerRangeController.jsx


-      startDate = day;
+    if (!range) {


this seems like it should be an else attached to the previous conditional block?

ljharb · 2017-12-06T00:13:32Z

src/components/DayPickerRangeController.jsx

-          modifiers = this.deleteModifierFromRange(modifiers, startDate, endSpan, 'hovered-span');
-        }
+      if (range && (range.before || range.after)) {
+        const start = range.before ? range.before(day.clone()) : day.clone();


this code seems repeated; can it live in somewhere shared?

jakeclements · 2017-12-06T00:18:46Z

Appreciate the feedback @ljharb, super quick.
All sounds good to me!

coveralls · 2017-12-06T02:14:05Z

Coverage increased (+0.1%) to 86.839% when pulling 4108a1e on jakeclements:custom-range-picker into e92deb3 on airbnb:master.

coveralls · 2017-12-06T03:38:35Z

Coverage increased (+0.1%) to 86.839% when pulling 8b9ebb5 on jakeclements:custom-range-picker into e92deb3 on airbnb:master.

ljharb

Thanks, LGTM, but I'm deferring to @majapw

majapw

This is starting to look really nice. Thank you for adding tests. I have a couple of high-level questions/comments:

Do we want to bubble up this functionality to the DateRangePicker as well?
Does the range functionality essentially mean you select a startDate and endDate with one click and is that... dramatically different enough to require its own picker?
I find the name of the range prop and the getRangeDay method to be incredibly confusing. I think there's also some overloading of what the hovered modifier actually does. I think I need to mull over some other options but it doesn't really convey what the prop does. something like rangeModifiers might be better? I'm not sure. I can brainstorm some more but would appreciate some feedback as well there.

🎉 thanks so much for the contribution!

majapw · 2017-12-06T21:51:11Z

src/components/CalendarDay.jsx

@@ -140,6 +140,7 @@ class CalendarDay extends React.Component {
          styles.CalendarDay_container,
          isOutsideDay && styles.CalendarDay__outside,
          modifiers.has('today') && styles.CalendarDay__today,
+          modifiers.has('hovered') && styles.CalendarDay__hovered,


Does this end up affected normal usage? It looks like the styles are different. We've also been relying on pseudoselectors so far so it might be good to decouple this naming from the actual hovering on calendar days

I agree that this should be decoupled to let pseudo selectors handle the majority of the hovers.
Lets update this one once we've had a think about naming conventions.

majapw · 2017-12-06T21:51:30Z

src/components/CalendarDay.jsx

@@ -193,6 +194,12 @@ export default withStyles(({ reactDates: { color } }) => ({
    },
  },

+  CalendarDay__hovered: {
+    background: color.core.borderBright,


Does this color exist in the theme?

I've added borderBright to defaultTheme.js.
Do any other files require an update for this?

doh! i just missed it. :)

majapw · 2017-12-06T21:53:10Z

examples/DayPickerRangeControllerWrapper.jsx

 const propTypes = forbidExtraProps({
  // example props for the demo
  autoFocusEndDate: PropTypes.bool,
  initialStartDate: momentPropTypes.momentObj,
  initialEndDate: momentPropTypes.momentObj,
+  range: RangeShape,


Putting this in the example props section of these propTypes makes this seem like its not a part of the DayPickerRangeController component, can we move this down?

jakeclements · 2017-12-07T00:40:32Z

Thanks for taking the time to review @majapw!

Do we want to bubble up this functionality to the DateRangePicker as well?

While it's completely possible the functionality of it may be confusing. As far as I'm aware, the standard for the DateRangePicker is always to select the start and end date seperately (please correct me if I'm wrong here).
If we were to introduce this I think it'd be helpful to specify it with another prop identifier other than range.
It isn't immediately clear to me that this would create a single-click selection in this case.

Does the range functionality essentially mean you select a startDate and endDate with one click and is that... dramatically different enough to require its own picker?

That's correct. I've bundled the functionality into DayPickerRangeController because while the interaction is different, the outcome is still range selection.
If we were to create another picker I think that we may find ourselves repeating a lot of what has already been achieved in DayPickerRangeController.
We might be able to work on some of the terminology used to make this clearer. Open to hearing your thoughts here.

I find the name of the range prop and the getRangeDay method to be incredibly confusing.

Naming things is hard, I agree. Let's work on this.

I think there's also some overloading of what the hovered modifier actually does. ... something like rangeModifiers might be better?

Good call, I can update this so it doesn't affect normal hovering. I'll add a modifier that follows whatever we decide on for our naming conventions.

Happy to hear your feedback on the above.
I'll have more of a think about how we could word the language to make some of this functionality clearer and will add any suggestions as comments here.

jakeclements · 2017-12-07T03:51:54Z

Hey @majapw, what is your opinion on the following;
In place of range we could pass selectRangeOnClick. I think this conveys what the prop does in a much clearer manner.
It would also allow us to bubble up functionality to DateRangePicker (hopefully) without causing confusion.

<DateRangePicker
  {...props}
  selectRangeOnClick={{
    beforeSelected: fn,
    afterSelected: fn,
  }}
/>

In place of the hovered modifier I would propose something along the lines of active-range, or hovered-range if active is too closely related to selected.

coveralls · 2017-12-07T04:56:51Z

Coverage increased (+0.1%) to 86.839% when pulling 82df46c on jakeclements:custom-range-picker into f0eff4f on airbnb:master.

coveralls · 2017-12-07T05:31:38Z

Coverage increased (+0.1%) to 86.839% when pulling 2f96358 on jakeclements:custom-range-picker into f0eff4f on airbnb:master.

circlingthesun · 2017-12-19T21:47:11Z

Ping?

jakeclements · 2017-12-27T00:38:41Z

@circlingthesun, I'd be keen to continue the discussion and development on this issue in the new year. If you have any suggestions as to how the props could be named/structured to suit your applications I'd be interested to hear it.

circlingthesun · 2017-12-27T01:27:51Z

@jakeclements, I really like this PR. So I'd be keen to help things move along.

I was looking to replace a custom date picker widget when I saw this. I need to be able to select an entire week by clicking on any day of said week.

I'd agree with @majapw that getRangeDay is a bit confusing (more so than range). Perhaps use getDateOffset?

Instead of range selectRangeOnClick or range, what if we use two props called startDateOffset and endDateOffset. This way the API is more flat and you don't need to provide a NOOP function if you don't want to offset the start or end?

Thoughts?

jakeclements · 2017-12-27T02:18:59Z

Thanks for jumping in @circlingthesun, I like this.
offset as the core language of this functionality sits well with me in differentiating it from the normal use of date-range.
Following this we could also create hovered-offset as the modifier.

In terms of selectRangeOnClick, it's currently possible to pass a single start or end into the object to create one offset.
I don't have a strong opinion on if this would be better suited to be passed as single object vs two props, happy to follow @majapw and yourself on this.

majapw · 2017-12-27T03:07:53Z

I'll take a look tonight or tomorrow. :) I just got back from vacay. Offset seems much clearer to me as the core word to use. Maybe even `getSelectedDateOffset`?

majapw

Looks great to me!

Can we delete the remaining range related stuff (RangeShape, getRangeDay) and rebase on the latest master?

It might make sense to squash down the commits as well (and will also make rebasing easier).

majapw · 2018-01-18T19:48:47Z

examples/DayPickerRangeControllerWrapper.jsx

@@ -31,6 +35,7 @@ const propTypes = forbidExtraProps({
  orientation: ScrollableOrientationShape,
  withPortal: PropTypes.bool,
  initialVisibleMonth: PropTypes.func,
+  range: RangeShape,


offsetRange maybe? are we still using this?

Replaced with startDateOffset/endDateOffset

majapw · 2018-01-18T19:54:04Z

src/shapes/RangeShape.js

@@ -0,0 +1,14 @@
+import PropTypes from 'prop-types';


Can we delete this?

majapw · 2018-01-18T19:54:32Z

src/utils/getRangeDay.js

@@ -0,0 +1,6 @@
+const defaultModifier = day => day;


Do we use this?

majapw · 2018-01-18T19:55:27Z

test/components/DayPickerRangeController_spec.jsx

@@ -1476,6 +1476,54 @@ describe('DayPickerRangeController', () => {
        );
      });
    });
+
+    describe('props.range', () => {


range => startDateOffset/endDateOffset

jakeclements · 2018-01-18T22:12:15Z

Thanks @majapw, I kinda left this half finished yesterday. Will wrap it up today.

coveralls · 2018-01-19T00:37:35Z

Coverage increased (+0.09%) to 86.417% when pulling b32f6a2 on jakeclements:custom-range-picker into 05b17b0 on airbnb:master.

coveralls · 2018-01-19T00:42:42Z

Coverage increased (+0.09%) to 86.417% when pulling ee58f3a on jakeclements:custom-range-picker into 05b17b0 on airbnb:master.

jakeclements · 2018-01-19T00:43:33Z

@majapw, this has been updated, squash & rebased. Please let me know if there are any other required changes.

coveralls · 2018-01-19T00:48:57Z

Coverage increased (+0.09%) to 86.417% when pulling ee58f3a on jakeclements:custom-range-picker into 05b17b0 on airbnb:master.

seems legit, but i'll defer to maja and erin

majapw

I'm a bit worried about the edge cases where you only define the start or the end of the offset. Can you confirm that those look good as well?

Otherwise the code is looking solid!

majapw · 2018-01-19T00:50:44Z

src/components/DayPickerRangeController.jsx

@@ -170,6 +176,7 @@ export default class DayPickerRangeController extends React.Component {
      'last-in-range': day => this.isLastInRange(day),
      hovered: day => this.isHovered(day),
      'hovered-span': day => this.isInHoveredSpan(day),
+      'hovered-offset': day => this.isInHoveredSpan(day),


Do we use the same method for hovered-span and for hovered-offset? does that cause any issues?

majapw · 2018-01-19T00:53:30Z

src/components/DayPickerRangeController.jsx

+          end,
+        };
+
+        if (this.state.dateOffset && this.state.dateOffset.start && this.state.dateOffset.end) {


what about the scenario where we only have dateOffset.start or dateOffset.end?

majapw · 2018-01-19T00:54:02Z

src/components/DayPickerRangeController.jsx

-        }
+      if (hasOffset) {
+        const start = getSelectedDateOffset(startDateOffset, day);
+        const end = getSelectedDateOffset(endDateOffset, day, rangeDay => rangeDay.add(1, 'day'));


Can you explain the need for the function here?

majapw · 2018-01-19T00:55:00Z

src/components/DayPickerRangeController.jsx

    if (this.isTouchDevice || !hoverDate) return;

    let modifiers = {};
    modifiers = this.deleteModifier(modifiers, hoverDate, 'hovered');

+    if (dateOffset) {


do we need the checks for dateOffset.start or dateOffset.end here?

majapw · 2018-01-19T01:01:59Z

Okay, I checked it out and it looks really good! Thanks so much for all the hard work! :)

coveralls · 2018-01-19T01:09:21Z

Coverage increased (+0.09%) to 86.417% when pulling bc338d2 on jakeclements:custom-range-picker into 05b17b0 on airbnb:master.

jakeclements · 2018-01-19T01:12:26Z

Thank you @majapw and @ljharb for trawling through my code and taking the time to review!

jonricaurte · 2018-02-16T19:22:53Z

@jakeclements is it possible to have the selection be week to week? Like a start week and an end week? So when I click a week it will show the start date be the first date of the first selected week. Then when I hover over other weeks after the first week it will fill in all the weeks in between the first selected week and the week I am hovering over. Then when I select a second week the end date will be the last day of the second selected week and all weeks in between the first selected week and the second selected week will be filled in. Thanks!

jonricaurte · 2018-02-16T19:24:35Z

This would be using "with current week range selection" in the storybook.

jakeclements · 2018-02-19T03:06:08Z

@jonricaurte this functionality unfortunately isn't covered by this PR. It should be possible based on the work completed here and some modification to the existing codebase. It's probably worth opening an issue to see if this is something the Airbnb team is interested in including.

jonricaurte · 2018-02-21T07:39:38Z

@jakeclements Thanks! Opened the following issue to track this:

#1035

ljharb previously requested changes Dec 6, 2017

View reviewed changes

ljharb reviewed Dec 6, 2017

View reviewed changes

ljharb added the semver-minor: new stuff Any feature or API addition. label Dec 6, 2017

ljharb requested review from majapw and erin-doyle December 6, 2017 06:50

majapw requested changes Dec 6, 2017

View reviewed changes

jakeclements force-pushed the custom-range-picker branch from 82df46c to 2f96358 Compare December 7, 2017 05:24

majapw requested changes Jan 18, 2018

View reviewed changes

jakeclements force-pushed the custom-range-picker branch 2 times, most recently from 2e8e02f to b32f6a2 Compare January 19, 2018 00:35

jakeclements force-pushed the custom-range-picker branch from b32f6a2 to ee58f3a Compare January 19, 2018 00:41

Add date-offset picker

bc338d2

jakeclements force-pushed the custom-range-picker branch from ee58f3a to bc338d2 Compare January 19, 2018 00:56

majapw approved these changes Jan 19, 2018

View reviewed changes

majapw merged commit 2db60a8 into react-dates:master Jan 19, 2018

jonricaurte mentioned this pull request Feb 21, 2018

Week to Week Picker #1035

Open

majapw mentioned this pull request Mar 9, 2018

Pick full week in DateRangePicker #1062

Closed

[Issue 349] Add single-click range picker #884

[Issue 349] Add single-click range picker #884

Conversation

jakeclements commented Dec 6, 2017

coveralls commented Dec 6, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakeclements commented Dec 6, 2017

coveralls commented Dec 6, 2017

coveralls commented Dec 6, 2017

ljharb left a comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakeclements commented Dec 7, 2017

jakeclements commented Dec 7, 2017

coveralls commented Dec 7, 2017

coveralls commented Dec 7, 2017

circlingthesun commented Dec 19, 2017

jakeclements commented Dec 27, 2017

circlingthesun commented Dec 27, 2017

jakeclements commented Dec 27, 2017

majapw commented Dec 27, 2017 via email • edited by ljharb

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakeclements commented Jan 18, 2018

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018

jakeclements commented Jan 19, 2018

coveralls commented Jan 19, 2018

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Jan 19, 2018

coveralls commented Jan 19, 2018

jakeclements commented Jan 19, 2018

jonricaurte commented Feb 16, 2018

jonricaurte commented Feb 16, 2018

jakeclements commented Feb 19, 2018

jonricaurte commented Feb 21, 2018

majapw commented Dec 27, 2017 via email •

edited by ljharb