-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Issue 349] Add single-click range picker #884
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
@@ -18,6 +18,10 @@ const propTypes = forbidExtraProps({ | |||
autoFocusEndDate: PropTypes.bool, | |||
initialStartDate: momentPropTypes.momentObj, | |||
initialEndDate: momentPropTypes.momentObj, | |||
range: PropTypes.shape({ | |||
before: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, at least one is required, I can make an update here.
@@ -39,6 +39,10 @@ const propTypes = forbidExtraProps({ | |||
startDate: momentPropTypes.momentObj, | |||
endDate: momentPropTypes.momentObj, | |||
onDatesChange: PropTypes.func, | |||
range: PropTypes.shape({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please store this propType somewhere reusable, so it can be shared wherever it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the entire statement can't fit on one line, each property needs to be on its own line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if (focusedInput === START_DATE) { | ||
onFocusChange(END_DATE); | ||
if (range) { | ||
startDate = range.before ? range.before(day.clone()) : day.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's 4 clones here; are all 4 needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not, I'll clean this up.
|
||
startDate = day; | ||
if (!range) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it should be an else
attached to the previous conditional block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You bet
modifiers = this.deleteModifierFromRange(modifiers, startDate, endSpan, 'hovered-span'); | ||
} | ||
if (range && (range.before || range.after)) { | ||
const start = range.before ? range.before(day.clone()) : day.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code seems repeated; can it live in somewhere shared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Appreciate the feedback @ljharb, super quick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM, but I'm deferring to @majapw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thegetRangeDay
method to be incredibly confusing. I think there's also some overloading of what thehovered
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!
src/components/CalendarDay.jsx
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/components/CalendarDay.jsx
Outdated
@@ -193,6 +194,12 @@ export default withStyles(({ reactDates: { color } }) => ({ | |||
}, | |||
}, | |||
|
|||
CalendarDay__hovered: { | |||
background: color.core.borderBright, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this color exist in the theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added borderBright to defaultTheme.js.
Do any other files require an update for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh! i just missed it. :)
const propTypes = forbidExtraProps({ | ||
// example props for the demo | ||
autoFocusEndDate: PropTypes.bool, | ||
initialStartDate: momentPropTypes.momentObj, | ||
initialEndDate: momentPropTypes.momentObj, | ||
range: RangeShape, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thanks for taking the time to review @majapw!
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).
That's correct. I've bundled the functionality into DayPickerRangeController because while the interaction is different, the outcome is still range selection.
Naming things is hard, I agree. Let's work on this.
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. |
Hey @majapw, what is your opinion on the following; <DateRangePicker
{...props}
selectRangeOnClick={{
beforeSelected: fn,
afterSelected: fn,
}}
/> In place of the |
82df46c
to
2f96358
Compare
Ping? |
@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. |
@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 Instead of range Thoughts? |
Thanks for jumping in @circlingthesun, I like this. In terms of |
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`?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -31,6 +35,7 @@ const propTypes = forbidExtraProps({ | |||
orientation: ScrollableOrientationShape, | |||
withPortal: PropTypes.bool, | |||
initialVisibleMonth: PropTypes.func, | |||
range: RangeShape, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offsetRange maybe? are we still using this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with startDateOffset/endDateOffset
src/shapes/RangeShape.js
Outdated
@@ -0,0 +1,14 @@ | |||
import PropTypes from 'prop-types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete this?
src/utils/getRangeDay.js
Outdated
@@ -0,0 +1,6 @@ | |||
const defaultModifier = day => day; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use this?
@@ -1476,6 +1476,54 @@ describe('DayPickerRangeController', () => { | |||
); | |||
}); | |||
}); | |||
|
|||
describe('props.range', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
range => startDateOffset/endDateOffset
Thanks @majapw, I kinda left this half finished yesterday. Will wrap it up today. |
2e8e02f
to
b32f6a2
Compare
b32f6a2
to
ee58f3a
Compare
@majapw, this has been updated, squash & rebased. Please let me know if there are any other required changes. |
seems legit, but i'll defer to maja and erin
ee58f3a
to
bc338d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use the same method for hovered-span and for hovered-offset? does that cause any issues?
end, | ||
}; | ||
|
||
if (this.state.dateOffset && this.state.dateOffset.start && this.state.dateOffset.end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the scenario where we only have dateOffset.start or dateOffset.end?
} | ||
if (hasOffset) { | ||
const start = getSelectedDateOffset(startDateOffset, day); | ||
const end = getSelectedDateOffset(endDateOffset, day, rangeDay => rangeDay.add(1, 'day')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the need for the function here?
if (this.isTouchDevice || !hoverDate) return; | ||
|
||
let modifiers = {}; | ||
modifiers = this.deleteModifier(modifiers, hoverDate, 'hovered'); | ||
|
||
if (dateOffset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the checks for dateOffset.start or dateOffset.end here?
Okay, I checked it out and it looks really good! Thanks so much for all the hard work! :) |
@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! |
This would be using "with current week range selection" in the storybook. |
@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. |
@jakeclements Thanks! Opened the following issue to track this: |
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)
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