Fix blocked days when moment obj time isn't noon #310
Conversation
| @@ -160,6 +160,8 @@ export default class DayPickerRangeController extends React.Component { | |||
| if (focusedInput !== END_DATE) return false; | |||
|
|
|||
| if (startDate) { | |||
| if (minimumNights === 1) return isSameDay(day, startDate); | |||
There was a problem hiding this comment.
This doesn't seem to be an issue that would just be relegated to minimumNights not equaling 1... It seems like it would cause problems for any minimumNights value, no?
There was a problem hiding this comment.
Oh you're right. I just tested this with my use case, which was without a minimumNights prop (so default 1). Will look into a more robust fix
There was a problem hiding this comment.
So I think the helper methods (isSameDay, etc.) all compare both dates at midnight. If we were to change the dayDiff line to:
const dayDiff = day.clone().startOf('day').diff(startDate.clone().startOf('day'), 'days');
that should work.
There was a problem hiding this comment.
I was trying a similar approach just now, but figured I'd describe it here first. I was worried about clone()'s performance, so I was doing dayDiff = day.day() - startDate.day(). Does this look better to you?
There was a problem hiding this comment.
That did it, also works for other values of minimum nights.
| @@ -303,6 +303,19 @@ describe('DayPickerRangeController', () => { | |||
| ); | |||
| expect(wrapper.instance().doesNotMeetMinimumNights(testDate)).to.equal(false); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
New test fails without above change
| @@ -163,7 +163,7 @@ export default class DayPickerRangeController extends React.Component { | |||
| if (focusedInput !== END_DATE) return false; | |||
|
|
|||
| if (startDate) { | |||
| const dayDiff = day.diff(startDate, 'days'); | |||
| const dayDiff = day.day() - startDate.day(); | |||
There was a problem hiding this comment.
This is not going to work over week borders as it will only return 0-6 and .date() and .dayOfYear() have similar problems over month and year boundaries respectively. I think unfortunately, something like I recommended earlier with comparing the startOf both days seems the most reasonable. Alternatively, because you are guaranteed that day will always be at noon, you could set startDate.hour('12) and compare those, although that is more brittle.
There was a problem hiding this comment.
Ugh, I meant date() which gets date of month and not day(). But that has the same issue with year boundaries. I then tried comparing both .year() and then comparing .dayOfYear(), but it seems like this is a lot less performant than the clone strategy, which surprised me. So looks like yes, that is the way to go.
For what it's worth, I also tested skipping setting day, and it's quite a bit better. What do you think?
I can go ahead with whichever strategy is the most ideal.
Thanks!
There was a problem hiding this comment.
Nice. Yeah, let's skip doing it for day, but because of https://github.com/airbnb/react-dates/blob/master/src/utils/getCalendarMonthWeeks.js#L5, let's actually do
day.diff(startDate.clone().startOf('day').hour(12), 'days');
just to cover all possible edge cases
majapw
added
the
semver-patch: fixes/refactors/etc
|
So it looks like this is failing because the spec has this definition of 'today':
Changing it to reflect the internal values in the actual picker (noon) fixes the issue. However, I was just hoping for a sanity check on that test change. |
| @@ -12,7 +12,7 @@ import isInclusivelyAfterDay from '../../src/utils/isInclusivelyAfterDay'; | |||
|
|
|||
| import { START_DATE, END_DATE } from '../../constants'; | |||
|
|
|||
| const today = moment().startOf('day'); | |||
| const today = moment().startOf('day').hours(12); | |||
There was a problem hiding this comment.
This makes sense. Maybe add a comment about how this is due to the fact that all moment objects passed to interaction handlers from CalendarDay are set to noon. Not necessary but could be nice.
There was a problem hiding this comment.
That 'today' is in three other specs. For consistency I think it would be better to bring them all in line with this. I can comment on each.
|
just spend about hour and half finding why my app was broken and i need to say this is stupid (and i don't see this documented anywhere) so if for example i take values from api/server i need to set hours to 12 in order to get startDate correctly highlighted in UI |
References #306.
Currently, the logic for checking if days are blocked by looking at number of days difference in
doesNotMeetMinimumNights(). However, the day difference rounds down to zero if the time difference is anything < 24hr.This patch checks if
minimumNights === 1and if so usesisSameDay()instead.I looked through the specs and couldn't figure out a good place to add a check for this base case, and would appreciate suggestions.
Before
(Note that after adding 5 minutes, the next day is now blocked)
After
(Day is not blocked after adding 5 minutes)
(Looking @ the console output, we see that the date has successfully been selected)