Conversation
majapw
added
the
semver-patch: fixes/refactors/etc
majapw
force-pushed
the
maja-fix-issue-where-range-doesnt-clear-on-invisible-month
branch
2 times, most recently
from
e9a3485 to
a479c4f
Compare
| const iso = toISODateString(day); | ||
|
|
||
| let updatedDaysAfterAddition = { ...updatedDays }; | ||
| if (enableOutsideDays) { |
There was a problem hiding this comment.
@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.
@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.
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 CalendarMonths 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.
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.
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! |
majapw
force-pushed
the
maja-fix-issue-where-range-doesnt-clear-on-invisible-month
branch
from
a479c4f to
2953125
Compare
|
|
||
| let currentMonth = firstVisibleMonth; | ||
| let numberOfMonths = numberOfVisibleMonths; | ||
| if (orientation !== VERTICAL_SCROLLABLE) { |
There was a problem hiding this comment.
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.
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.
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.
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
Per discussion, LGTM for now, and there should be a followup to consolidate all the repeated code <3
majapw
deleted the
maja-fix-issue-where-range-doesnt-clear-on-invisible-month
branch
3 participants
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
enableOutsideDaysprop, but I think this solution is holistic and does not have outside side effects. The changes toaddModifierhelp to update invisible days if you happen to click on an "outside day".PTAL @airbnb/webinfra @acr13