Change month height calculation to be based on number of weeks instead of DOM calculations #1192

majapw · 2018-06-01T17:20:19Z

Hello friends!

This is a precursor to getting #1106 merged in. I separated it out into its own change because it was fairly involved and I didn't want to pollute the code-change in #1106 with something not entirely related.

Basically, when playing around with the month/year selection I found an issue where if you jumped to a month of a different height (5 week month => 6 week month), the height wouldn't automatically adjust as expected. When I started trying to fix the issue, it became apparent that it wasn't possible to do without jank using the existing paradigms.

Before:

On render, each CalendarMonth component (including the two invisible ones for animation) would measure its own height in the DOM.
Each CalendarMonth would then call a callback prop, setMonthHeight, with this measured value.
The CalendarMonthGrid would add these values to an ordered array of CalendarMonth heights which it would then pass back up to the DayPicker via the setCalendarMonthHeights prop
Once the DayPicker would have all of the heights, it would calculate the expected height based on the max of the visible months and grow the calendar appropriately.
On month transitions, it would use this array of heights to grow or shrink the calendar appropriately while transitioning.

The important thing to note is that the only way it could change the height while transitioning was to use the self-calculations made by the invisible months that were already rendered to the DOM. When we want to transition to an arbitrary month or year, there's guaranteed to be jank in this system because we have to render the month before we know its height. The end result is that if you transition from July 2018 to July 2017, the 6-week month will be cut off before it grows. The other option is to hide the calendar in this interim, but that flash is also very apparent.

SO, this change is designed to amend that.

Now:

On the first render (and any time renderMonth changes), the CalendarMonth measures itself, subtracts out the height of the CalendarDay table (which is deterministic based on number of weeks and the daySize prop), and call the setMonthTitleHeight prop with the value.
The DayPicker receives this value and saves it in this.monthTitleHeight
The DayPicker also maintains an array of the number of weeks in each visible month.
On every month transition, the DayPicker uses the number of weeks in the month coming into view along with the daySize prop and the saved monthTitleHeight to calculate the expected height of the incoming month and adjusts the height appropriately.

I've tested this in both the vertical and the horizontal pickers and it seems pretty effective! Because we can pretty readily calculate the number of weeks in any incoming month, this should open the door to slick month transitions to any month. We also, tbh, could probably remove the invisible months from render until it's time to slide them in. That can wait for another time though.

Note, this does assume a pretty constant month title height, but tbh I think that was an implicit assumption before.

Would love some eyes! @ljharb @gabergg @GoGoCarl

These props that I'm changing are all what I would consider internal props (it would be weird if an outside consumer was relying on them for anything)... but are technically part of the API, so this change could be considered breaking. Idk.

TODO:
-[X] Write tests for getNumberOfCalendarMonthWeeks
-[X] reset month height when renderMonth changes

ljharb

Is test/utils/getNumberOfCalendarMonthWeeks_spec.js empty?

Yes, I think this is breaking.

ljharb · 2018-06-01T17:23:52Z

src/components/DayPicker.jsx

+    if (this.isVertical()) {
+      const calendarMonthWeeksHeight = this.calendarMonthWeeks[0] * (daySize - 1);
+      translationValue = monthTitleHeight + calendarMonthWeeksHeight + 1;
+    }

    if (this.isHorizontal()) {


this seems like it should be an else if

ljharb · 2018-06-01T17:28:24Z

src/components/DayPicker.jsx

-    let translationValue = this.isVertical() ? this.calendarMonthHeights[0] : calendarMonthWidth;
+    const newMonth = currentMonth.clone().subtract(1, 'month');
+    const numberOfWeeks = getNumberOfCalendarMonthWeeks(newMonth, this.getFirstDayOfWeek());
+    this.calendarMonthWeeks = [numberOfWeeks].concat(this.calendarMonthWeeks.slice(0, -1));


[numberOfWeeks, ...this.calendarMonthWeeks.slice(0, -1)] ?

ljharb · 2018-06-01T17:28:49Z

src/components/DayPicker.jsx

+
+    let translationValue;
+    if (this.isVertical()) {
+      const calendarMonthWeeksHeight = this.calendarMonthWeeks[0] * (daySize - 1);


why does this only need the first week?
also, could it do const [firstWeek] = this.calendarMonthWeeks;?

calendarMonthWeeks is an array of the number of weeks in each of the calendarmonths. The vertical picker scrolls forward by the amount of weeks in the first (currently invisible) month.

ljharb · 2018-06-01T17:29:43Z

src/components/DayPicker.jsx

+    let translationValue;
+
+    if (this.isVertical()) {
+      const calendarMonthWeeksHeight = this.calendarMonthWeeks[0] * (daySize - 1);


SO there is a difference between the two. in the one the weeks array has already been updated to the new state; in the other it hasnt'.

ljharb · 2018-06-01T17:30:07Z

src/components/DayPicker.jsx

+
+    if (this.isVertical()) {
+      const calendarMonthWeeksHeight = this.calendarMonthWeeks[0] * (daySize - 1);
+      translationValue = -(monthTitleHeight + calendarMonthWeeksHeight + 1);


this "is vertical" logic is repeated in at least 2 places; can it be shared?

it's not quite the same

ljharb · 2018-06-01T17:30:40Z

src/components/DayPicker.jsx

+      translationValue = -(monthTitleHeight + calendarMonthWeeksHeight + 1);
+    }
+
+    this.calendarMonthWeeks = this.calendarMonthWeeks.slice(1).concat([numberOfWeeks]);


const [, ...rest] = this.calendarMonthWeeks; this.calendarMonthWeeks = [...rest, numberOfWeeks];?

coveralls · 2018-06-01T17:39:30Z

Coverage increased (+0.3%) to 85.179% when pulling 0bd08cb on maja-calc-height-based-on-weeks-instead-of-DOM into 2e13fdc on master.

majapw

PTAL @ljharb

ljharb

o/

gabergg · 2018-06-01T23:05:50Z

src/components/DayPicker.jsx

-    this.calendarMonthGridHeight = Math.max(0, ...visibleCalendarMonthHeights) + MONTH_PADDING;
-    this.setState({ hasSetHeight: true });
+    this.calendarMonthWeeks = [];
+    for (let i = 0; i < numberOfMonths; i += 1) {


I wonder if we could tweak this to in the same way to capture months before and after the current month? And then on prev/next click, we could update the neighbor months asynchronously, the way that we might preload images in a carousel? I could be misunderstanding the logic too.

gabergg · 2018-06-01T23:07:44Z

src/components/DayPicker.jsx


    if (e) e.preventDefault();

-    let translationValue = this.isVertical() ? this.calendarMonthHeights[0] : calendarMonthWidth;
+    const newMonth = currentMonth.clone().subtract(1, 'month');
+    const numberOfWeeks = getNumberOfCalendarMonthWeeks(newMonth, this.getFirstDayOfWeek());


I mostly think it would be cool to reduce the work done on prev/next click. Wondering if we can pre-calculate and key the months somehow, or even memoize getNumberOfCalendarMonthWeeks or something so that we don't have to recalculate week count and shift calendarMonthWeeks on each transition.

gabergg

LGTM. My only comments are potential optimizations that may or may not make sense and/or matter

GoGoCarl

@majapw Looks great, the approach makes sense, and works as advertised!

Looking forward to #1106, the onMonthChange and onYearChange functions will need to have code added to conditionally adjust the height, as you did with onPrevMonthClick and onNextMonthClick. I was able to verify this behavior by adding this function:

maybeAdjustDayPickerHeight(newMonth) {
    const { daySize } = this.props;
    const { monthTitleHeight } = this.state;

    const numberOfWeeks = getNumberOfCalendarMonthWeeks(newMonth, this.getFirstDayOfWeek());
    this.calendarMonthWeeks = [numberOfWeeks, ...this.calendarMonthWeeks.slice(0, -1)];

    if (this.isHorizontal()) {
      const calendarMonthWeeksHeight = Math.max(0, ...this.calendarMonthWeeks) * (daySize - 1);
      const newMonthHeight = monthTitleHeight + calendarMonthWeeksHeight + 1;
      this.adjustDayPickerHeight(newMonthHeight);
    }
  }

And calling it in onMonthChange and onYearChange just before the setState call, and the heights were adjusted as I'd expect. There may (or may not) need to be more done there, and the transitions are still the same (0.0001), but it seems to work nicely!

For a couple quick checks/verification, I tested with two cases:

Jumping from June 2018 to January 2018 to September 2018 -- this used to clip the month height, now it does not
Jumping from June 2018 to June 2019 to June 2018 -- sizes up and down correctly!

That said, perhaps that maybeAdjustDayPickerHeight (based on code from onPrevMonthClick) could also be called from onPrevMonthClick and onNextMonthClick? I know one of them is slightly different, but perhaps?

Anyway, all that is for later, this PR as-is LGTM.

…d of DOM calculations

…in the click handlers

majapw · 2018-06-05T00:02:43Z

Sweet! Okay, I deferred some of the heavier cloning logic from onPrevMonthClick/onNextMonthClick to updateStateAfterTransition. @GoGoCarl I think you're right that we'll likely want to abstract out that logic a bit in the month/year change. We can address that in your PR though, I think.

I'm gonna merge this in once everything passes.

majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Jun 1, 2018

ljharb reviewed Jun 1, 2018

View reviewed changes

ljharb added semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. and removed semver-patch: fixes/refactors/etc Anything that's not major or minor. labels Jun 1, 2018

majapw force-pushed the maja-calc-height-based-on-weeks-instead-of-DOM branch 2 times, most recently from 319a3fa to 6f1da2e Compare June 1, 2018 21:02

majapw commented Jun 1, 2018

View reviewed changes

majapw force-pushed the maja-calc-height-based-on-weeks-instead-of-DOM branch from 6f1da2e to 8dec891 Compare June 1, 2018 21:24

ljharb approved these changes Jun 1, 2018

View reviewed changes

gabergg reviewed Jun 1, 2018

View reviewed changes

gabergg approved these changes Jun 1, 2018

View reviewed changes

GoGoCarl approved these changes Jun 2, 2018

View reviewed changes

majapw added 2 commits June 4, 2018 16:57

Change month height calculation to be based on number of weeks instea…

8e71edf

…d of DOM calculations

Defer moment object cloning to updateStateAfterTransition instead of …

0bd08cb

…in the click handlers

majapw force-pushed the maja-calc-height-based-on-weeks-instead-of-DOM branch from 4605dde to 0bd08cb Compare June 4, 2018 23:57

majapw merged commit ef42df2 into master Jun 5, 2018

majapw deleted the maja-calc-height-based-on-weeks-instead-of-DOM branch June 5, 2018 01:38

GoGoCarl mentioned this pull request Jun 14, 2018

[Issue 25] Unopinionated month and year selection support #1106

Merged

majapw mentioned this pull request Jul 3, 2018

Update conditions when adjustDayPickerHeight is called #1241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change month height calculation to be based on number of weeks instead of DOM calculations #1192

Change month height calculation to be based on number of weeks instead of DOM calculations #1192

majapw commented Jun 1, 2018 •

edited

ljharb left a comment

ljharb Jun 1, 2018

ljharb Jun 1, 2018

ljharb Jun 1, 2018

majapw Jun 1, 2018

ljharb Jun 1, 2018

majapw Jun 1, 2018

ljharb Jun 1, 2018

majapw Jun 1, 2018

ljharb Jun 1, 2018

majapw Jun 1, 2018

coveralls commented Jun 1, 2018 •

edited

majapw left a comment

ljharb left a comment

gabergg Jun 1, 2018

gabergg Jun 1, 2018

gabergg left a comment

GoGoCarl left a comment

majapw commented Jun 5, 2018

Change month height calculation to be based on number of weeks instead of DOM calculations #1192

Change month height calculation to be based on number of weeks instead of DOM calculations #1192

Conversation

majapw commented Jun 1, 2018 • edited

ljharb 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

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

Choose a reason for hiding this comment

coveralls commented Jun 1, 2018 • edited

majapw left a comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabergg left a comment

Choose a reason for hiding this comment

GoGoCarl left a comment

Choose a reason for hiding this comment

majapw commented Jun 5, 2018

majapw commented Jun 1, 2018 •

edited

coveralls commented Jun 1, 2018 •

edited