-
-
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 issue where range doesn't clear on invisible months #575
Fix issue where range doesn't clear on invisible months #575
Conversation
e9a3485
to
a479c4f
Compare
const iso = toISODateString(day); | ||
|
||
let updatedDaysAfterAddition = { ...updatedDays }; | ||
if (enableOutsideDays) { |
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.
@ljharb This first change is in the addModifier
method. The basic premise is that if outside days are enabled that means that days at the beginning and end of the month will exist as a part of two separate CalendarMonth
objects. As a result if you click on a day, you may want to add modifiers to more than one month instance in the state. This is what this branch of the conditional accomplishes.
const iso = toISODateString(day); | ||
|
||
let updatedDaysAfterAddition = { ...updatedDays }; | ||
if (enableOutsideDays) { |
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.
@ljharb This first change is in the addModifier
method. The basic premise is that if outside days are enabled that means that days at the beginning and end of the month will exist as a part of two separate CalendarMonth
objects. As a result if you click on a day, you may want to add modifiers to more than one month instance in the state. This is what this branch of the conditional accomplishes.
const iso = toISODateString(day); | ||
|
||
let updatedDaysAfterDeletion = { ...updatedDays }; | ||
if (enableOutsideDays) { |
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 addition to the deleteModifier
method is very similar to what happens in addModifier
. Namely, for outside days, sometimes a selected day might be in two CalendarMonth
s and we need to remove the modifiers in both cases.
|
||
let currentMonth = firstVisibleMonth; | ||
let numberOfMonths = numberOfVisibleMonths; | ||
if (orientation !== VERTICAL_SCROLLABLE) { |
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.
currentMonth
and numberOfMonths
only apply to the visible months and not to those hidden for animation. However, we want to make sure modifiers are updates even in the invisible months so we expand the range here. ... I didn't think we needed this when adding modifiers but on second thought I think we do (there's like this edge case where you select a start date, scroll it off screen, select and end date and scroll back).
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 if
logic (ie, getting currentMonth
and numberOfMonths
) is repeated in DayPickerRangeController, and in SingleDatePicker. Could we move it to a shared function instead?
@ljharb I added comments here as well to help guide the review! |
a479c4f
to
2953125
Compare
|
||
let currentMonth = firstVisibleMonth; | ||
let numberOfMonths = numberOfVisibleMonths; | ||
if (orientation !== VERTICAL_SCROLLABLE) { |
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 if
logic (ie, getting currentMonth
and numberOfMonths
) is repeated in DayPickerRangeController, and in SingleDatePicker. Could we move it to a shared function instead?
Object.keys(visibleDays[monthKey]).indexOf(iso) > -1 | ||
)); | ||
|
||
updatedDaysAfterAddition = monthsToUpdate.reduce((days, monthIso) => { |
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 one or both branches of this if/else seem repeated in both components; could they also be 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.
I think it is complicated to share these methods because a lot of these variables depend on state and/or props. I think the dream scenario is eventually to have a withModifiers
HOC of some sort that could do a lot of this stuff on its own.
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.
Per discussion, LGTM for now, and there should be a followup to consolidate all the repeated code <3
Fix for #567
I think this is a more bullet-proof solution than #574
It took me quite a while to figure out how to get this working for the
enableOutsideDays
prop, but I think this solution is holistic and does not have outside side effects. The changes toaddModifier
help to update invisible days if you happen to click on an "outside day".PTAL @airbnb/webinfra @acr13