Fix blocked days when moment obj time isn't noon #310

timhwang21 · 2017-02-08T08:04:24Z

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 === 1 and if so uses isSameDay() 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)

coveralls · 2017-02-08T08:18:17Z

Coverage increased (+0.02%) to 86.52% when pulling d4bfb48 on timhwang21:fix-blocked-days-when-moment-not-noon into 4c2c604 on airbnb:master.

coveralls · 2017-02-08T17:01:08Z

Coverage increased (+0.02%) to 86.52% when pulling 00a8e45 on timhwang21:fix-blocked-days-when-moment-not-noon into 4c2c604 on airbnb:master.

majapw · 2017-02-09T23:56:48Z

src/components/DayPickerRangeController.jsx

@@ -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);


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?

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

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.

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?

That did it, also works for other values of minimum nights.

timhwang21 · 2017-02-10T01:04:37Z

test/components/DayPickerRangeController_spec.jsx

@@ -303,6 +303,19 @@ describe('DayPickerRangeController', () => {
            );
            expect(wrapper.instance().doesNotMeetMinimumNights(testDate)).to.equal(false);
          });
+


New test fails without above change

coveralls · 2017-02-10T01:13:54Z

Coverage remained the same at 86.501% when pulling 424d1db on timhwang21:fix-blocked-days-when-moment-not-noon into 3eafa08 on airbnb:master.

coveralls · 2017-02-10T01:22:50Z

Coverage remained the same at 86.501% when pulling 424d1db on timhwang21:fix-blocked-days-when-moment-not-noon into 3eafa08 on airbnb:master.

majapw · 2017-02-10T21:46:15Z

src/components/DayPickerRangeController.jsx

@@ -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();


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.

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!

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

LGTM!

timhwang21 · 2017-02-10T23:27:20Z

So it looks like this is failing because the spec has this definition of 'today':

const today = moment().startOf('day');

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.

majapw

Looks good to me!

majapw · 2017-02-10T23:32:40Z

test/components/DayPickerRangeController_spec.jsx

@@ -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);


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.

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.

coveralls · 2017-02-10T23:46:32Z

Coverage remained the same at 86.501% when pulling 87a5fa0 on timhwang21:fix-blocked-days-when-moment-not-noon into cfc7b16 on airbnb:master.

coveralls · 2017-02-11T00:04:24Z

Coverage remained the same at 86.501% when pulling 87a5fa0 on timhwang21:fix-blocked-days-when-moment-not-noon into cfc7b16 on airbnb:master.

thecotne · 2019-03-26T18:46:23Z

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

majapw reviewed Feb 9, 2017

View reviewed changes

timhwang21 commented Feb 10, 2017

View reviewed changes

majapw reviewed Feb 10, 2017

View reviewed changes

majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Feb 10, 2017

majapw approved these changes Feb 10, 2017

View reviewed changes

Fix blocked days when moment obj time isn't noon

87a5fa0

majapw merged commit 0ab2062 into react-dates:master Feb 11, 2017

timhwang21 deleted the fix-blocked-days-when-moment-not-noon branch February 11, 2017 00:28

thecotne mentioned this pull request Mar 27, 2019

startDate UI is broken if startDate time is not 12:00:00 #1599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix blocked days when moment obj time isn't noon #310

Fix blocked days when moment obj time isn't noon #310

timhwang21 commented Feb 8, 2017

coveralls commented Feb 8, 2017

coveralls commented Feb 8, 2017

majapw Feb 9, 2017

timhwang21 Feb 10, 2017

majapw Feb 10, 2017

timhwang21 Feb 10, 2017

timhwang21 Feb 10, 2017

timhwang21 Feb 10, 2017

coveralls commented Feb 10, 2017

coveralls commented Feb 10, 2017

majapw Feb 10, 2017

timhwang21 Feb 10, 2017

majapw Feb 10, 2017

majapw left a comment

timhwang21 commented Feb 10, 2017

majapw left a comment

majapw Feb 10, 2017

timhwang21 Feb 10, 2017

majapw Feb 10, 2017

coveralls commented Feb 10, 2017

coveralls commented Feb 11, 2017

thecotne commented Mar 26, 2019

Fix blocked days when moment obj time isn't noon #310

Fix blocked days when moment obj time isn't noon #310

Conversation

timhwang21 commented Feb 8, 2017

coveralls commented Feb 8, 2017

coveralls commented Feb 8, 2017

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

coveralls commented Feb 10, 2017

coveralls commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

timhwang21 commented Feb 10, 2017

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

coveralls commented Feb 10, 2017

coveralls commented Feb 11, 2017

thecotne commented Mar 26, 2019