-
-
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
Fix minimum nights availability issue #1259
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.
This being order-dependent seems kinda brittle. Can you add a test that would have caught this bug?
@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. |
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 |
cb4517d
to
547bb2c
Compare
} else { | ||
modifiers = this.deleteModifier(modifiers, momentObj, 'blocked-calendar'); | ||
} | ||
} | ||
|
||
if (isBlocked) { |
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 covering isDayBlocked and isOutsideRange, but this.isBlocked
also covers doesNotMeetMinimumNights
. should that get a test case also?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be reverted
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.
whoops! :) good catch.
@@ -673,7 +673,7 @@ describe('DayPickerRangeController', () => { | |||
}); | |||
}); | |||
|
|||
describe('focusedInput changed', () => { | |||
describe.only('focusedInput changed', () => { |
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.
also this
547bb2c
to
38422d7
Compare
38422d7
to
9c24607
Compare
'blocked-minimum-nights', | ||
); | ||
|
||
modifiers = this.addModifierToRange( |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw i think this might be covered by wrapper.setProps
? not a blocker tho
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 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({ |
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.
ditto
startDate={startDate} | ||
minimumNights={minimumNights} | ||
/>); | ||
wrapper.instance().componentWillReceiveProps({ |
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.
ditto
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 atcomponentWillReceiveProps
in 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