Fix minimum nights availability issue #1259
Conversation
majapw
added
the
semver-patch: fixes/refactors/etc
|
@lencioni that's a good idea. I may try to think of a way to make it less order dependent as well 🤔 I was also walking through the function step-by-step and I still don't really understand... why this is happening. :| so maybe I will revisit in addition to writing a test. |
majapw
force-pushed
the
maja-fix-min-nights-availability-issue
branch
from
b723add
to
cb4517d
Compare
|
@lencioni @ljharb I figured out the actual source of the issue: it was that when focus changed, we were applying the I added some tests that check for this specific situation and I also updated the |
majapw
force-pushed
the
maja-fix-min-nights-availability-issue
branch
from
cb4517d
to
547bb2c
Compare
| } else { | ||
| modifiers = this.deleteModifier(modifiers, momentObj, 'blocked-calendar'); | ||
| } | ||
| } | ||
|
|
||
| if (isBlocked) { |
There was a problem hiding this comment.
this is covering isDayBlocked and isOutsideRange, but this.isBlocked also covers doesNotMeetMinimumNights. should that get a test case also?
There was a problem hiding this comment.
That gets covered in a minimum nights specific code path because the triggers for re-calculating min nights are slightly different. It does get handled though!
| @@ -95,7 +95,7 @@ describe('DayPickerRangeController', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('#componentWillReceiveProps', () => { | |||
| describe.only('#componentWillReceiveProps', () => { | |||
There was a problem hiding this comment.
whoops! :) good catch.
| @@ -673,7 +673,7 @@ describe('DayPickerRangeController', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('focusedInput changed', () => { | |||
| describe.only('focusedInput changed', () => { | |||
majapw
force-pushed
the
maja-fix-min-nights-availability-issue
branch
from
547bb2c
to
38422d7
Compare
majapw
force-pushed
the
maja-fix-min-nights-availability-issue
branch
from
38422d7
to
9c24607
Compare
| 'blocked-minimum-nights', | ||
| ); | ||
|
|
||
| modifiers = this.addModifierToRange( |
There was a problem hiding this comment.
Here is where the blocked modifier will get added back if it has been removed by line 400 unnecessarily. @ljharb
|
@ljharb PTAL! I think I have addressed your comments. :) |
| day.add(1, 'day'); | ||
| } | ||
|
|
||
| wrapper.instance().componentWillReceiveProps({ |
There was a problem hiding this comment.
btw i think this might be covered by wrapper.setProps? not a blocker tho
There was a problem hiding this comment.
This is what I've been using in all the tests in this describe block, so if I wanted to update I'd probably do everything all at once at a later date. :)
| startDate={startDate} | ||
| minimumNights={minimumNights} | ||
| />); | ||
| wrapper.instance().componentWillReceiveProps({ |
| startDate={startDate} | ||
| minimumNights={minimumNights} | ||
| />); | ||
| wrapper.instance().componentWillReceiveProps({ |
Some strange behavior that we noticed on airbnb.com is that after the first render (e.g. when you change focus or change the selected date) on a calendar with minimum nights enabled, while the actual interactivity of the minimum nights days still updates correctly, the aria-label would be the opposite of what was expected. This meant that when the days were grayed out due to not satisfying the minimum nights requirement, the aria label would read them as available and when they were clickable, like when the start date was focused, the aria label would read them as unavailable.
Digging into this issue, it appeared to be related to how the
'blocked'modifier was being updated when the state changed. Looking atcomponentWillReceivePropsin theDayPickerRangeController, where most of the logic for modifiers lives.Specifically, the inconsistency happened when focus changed and would remain thereafter. I think that the minimum nights logic would sometimes remove a 'blocked' modifier or forget to add a 'blocked' modifier unintentionally, which would flip the aria label from available to unavailable.
This change in ordering seems to address the issue.
@ljharb @backwardok @sdjidjev @airbnb/webinfra PTAL