feat(datepicker): align multi-year-view based on minDate and maxDate #16018

jysoo · 2019-05-13T22:58:05Z

If minDate is set (and maxDate not set), let first page begin with minYear.
If maxDate is set, let last page end with maxYear,
and disable year(s) < minYear (if any) in first page.

Fixes #10646

mmalerba

This is awesome, thanks!! Only request: could you add a test that verifies the behavior for min set, max set, and both set?

jysoo · 2019-05-17T22:20:46Z

@mmalerba Cheers!

Splaktar

Thank you for the contribution! I only had a few minor nits.

src/material/datepicker/calendar.ts

src/material/datepicker/multi-year-view.ts

src/material/datepicker/calendar.ts

jelbourn · 2019-05-17T23:47:02Z

src/material/datepicker/calendar.ts

+  }
+
+  /** mod that handles case where first number is negative */
+  private _mod(a: number, b: number) {


nit: this can be an un-exported file-level function

src/material/datepicker/calendar.ts

@@ -69,11 +69,24 @@ export class MatCalendarHeader<D> {
    if (this.calendar.currentView == 'year') {
      return this._dateAdapter.getYearName(this.calendar.activeDate);
    }
+


jelbourn · 2019-05-18T00:18:54Z

src/material/datepicker/calendar.ts

+    if (this.calendar.maxDate) {
+      let maxYear = this._dateAdapter.getYear(this.calendar.maxDate);
+      minYear = maxYear - yearsPerPage + 1;
+    }


This block is repeated a few times; could you make it a shared function?

jelbourn · 2019-05-18T00:33:38Z

src/material/datepicker/multi-year-view.ts

+      minYear = maxYear - yearsPerPage + 1;
+    }
+    let activeOffset = this._mod((activeYear - minYear), yearsPerPage);
+    let minYearOfPage = activeYear - activeOffset;


It would also be good to deduplicate this logic a bit

jelbourn

For some reason GitHub lost one of my comments from the last pass

jelbourn · 2019-05-18T00:37:50Z

src/material/datepicker/calendar.ts

@@ -149,8 +162,21 @@ export class MatCalendarHeader<D> {
      return this._dateAdapter.getYear(date1) == this._dateAdapter.getYear(date2);
    }
    // Otherwise we are in 'multi-year' view.
-    return Math.floor(this._dateAdapter.getYear(date1) / yearsPerPage) ==
-        Math.floor(this._dateAdapter.getYear(date2) / yearsPerPage);
+    let minYear = 0;


I would tweak this slightly:

// We want a range years such that we maximize the number of // enabled dates visible at once. This prevents issues where the minimum year // is the last year of a page or the maximum year is the first item of a page. // We pick a "starting" year such that either the maximum year would be at the end // or the minimum year would be at the begining of a page. let startingYear = 0; if (this.calendar.maxDate) { let maxYear = this._dateAdapter.getYear(this.calendar.maxDate); startingYear = maxYear - yearsPerPage + 1; } else if (this.calendar.minDate) { startingYear = this._dateAdapter.getYear(this.calendar.minDate); } // When the multi-year view is first opened, the active year will be in view. // So we compute how many years are between the active year and the *slot* where our // "starting" year year will render when paged into view. const activeYear = this._dateAdapter.getYear(this.calendar.activeDate); const activeOffset = this._mod((activeYear - startingYear), yearsPerPage); // The offset from the active year to the "slot" for the starting year is the // *actual* first rendered year in the multi-year view, and the lasy year is // just one yearsPerPage away. const minYearOfPage = activeYear - activeOffset; const maxYearOfPage = minYearOfPage + yearsPerPage - 1; const firstYearInView = this._dateAdapter.getYearName( this._dateAdapter.createDate(minYearOfPage, 0, 1)); const lastYearInView = this._dateAdapter.getYearName( this._dateAdapter.createDate(maxYearOfPage, 0, 1)); return `${firstYearInView} \u2013 ${lastYearInView}`;

Adds some explanatory comments

Changes let to const where appropriate

Combines two separate if blocks into an if-elseif

Changes minDate to startingDate since it's not necessarily the minDate

jysoo · 2019-05-19T20:00:44Z

Thank you for the comments! :)

Modifications to originally existing code: the === sign that Michael mentioned, as well as plugging in the minYearOfPage and maxYearOfPage directly into the periodButtonText string (instead of getYearName after creating Date from Year).
Shared logic (I simply exported the getActiveOffset and isSameMultiYearView in multi-year-view.ts so that calendar-ts can use them as well):
getStartingYear is used for:

getActiveOffset (which is used for _getActiveCell as well as minYearOfPage)
isSameMultiYearView

Please let me know if you have any suggestions for improving the implementation.

Splaktar

LGTM other than a couple minor nits.

src/material/datepicker/multi-year-view.ts

jelbourn · 2019-05-20T18:07:08Z

src/material/datepicker/multi-year-view.ts

@@ -39,6 +39,43 @@ export const yearsPerPage = 24;

 export const yearsPerRow = 4;

+/** Gets remainder that is non-negative, even if first number is negative */
+function euclideanModulo (a: number, b: number): number {


Could you move these helper functions to the bottom of the file? We usually organize them that way

jelbourn · 2019-05-20T18:11:52Z

src/material/datepicker/multi-year-view.ts

+function getStartingYear(minDate: any, maxDate: any): number {
+  let startingYear = 0;
+  if (maxDate) {
+    const maxYear = maxDate.getFullYear();


It looks like these helper functions aren't going through the adapter any more, so they're assuming native JS Date (here and in getActiveOffset). You could make these private class methods so you still have access to the adapter.

You also would want to avoid using any in the types because it masks issues like this

jelbourn · 2019-05-20T18:13:28Z

src/material/datepicker/multi-year-view.ts

+ * So we compute how many years are between the active year and the *slot* where our
+ * "startingYear" will render when paged into view.
+ */
+export function getActiveOffset(activeDate: any, minDate: any, maxDate: any): number {


If there are new exported helper functions like this, then they need to be omitted from datepicker/public-api.ts in order to avoid introducing them as public APIs.

jelbourn · 2019-05-20T18:22:04Z

src/material/datepicker/multi-year-view.ts

+  const year1 = date1.getFullYear();
+  const year2 = date2.getFullYear();
+  const startingYear = getStartingYear(minDate, maxDate);
+  return Math.floor( (year1 - startingYear) / yearsPerPage) ===


Extra spaces inside parens

jelbourn · 2019-05-20T18:22:54Z

src/material/datepicker/multi-year-view.ts

    this._years = [];
    for (let i = 0, row: number[] = []; i < yearsPerPage; i++) {
-      row.push(activeYear - activeOffset + i);
+      row.push( minYearOfPage + i);


Extra space

jelbourn · 2019-06-05T17:33:42Z

src/material/datepicker/calendar.ts

+    return this._isSameMultiYearView(date1, date2);
+  }
+
+  private _isSameMultiYearView(date1: D, date2: D): boolean {


These functions are still duplicated; it would be better to extract them to one place where they accept a dateAdapter as an argument rather than defining them in both classes.

jysoo · 2019-06-14T14:32:17Z

@mmalerba @jelbourn @Splaktar
I'm not familiar with this; may I have some help/advice with the CircleCI 'Too long with no output (exceeded 10m0s)' issue during the browserstack test? Thanks.

jelbourn

LGTM aside from one small formatting nit

jelbourn · 2019-06-13T21:29:19Z

src/material/datepicker/calendar.ts

+import {MatMultiYearView,
+        yearsPerPage,
+        getActiveOffset,
+        isSameMultiYearView} from './multi-year-view';


This formatting seems a bit off

Splaktar · 2019-06-14T17:09:21Z

I'm not familiar with this; may I have some help/advice with the CircleCI 'Too long with no output (exceeded 10m0s)' issue during the browserstack test? Thanks.

@jysoo that's likely just a flakey test or a issue with CircleCI and not likely a problem with your changes.

It looks like Safari on macOS isn't launching properly and timing out. It looks like your branch is still based on 8.0.0-rc.4. Can you please rebase on master?

If minDate is set (and maxDate not set), let first page begin with minYear. If maxDate is set, let last page end with maxYear, and disable year(s) < minYear (if any) in first page. Fixes angular#10646

…e and maxDate

… maxDate

refactor with private methods feat(datepicker): align multi-year-view based on minDate and maxDate

…nd maxDate

mmalerba · 2019-06-14T17:30:48Z

@jysoo I rebased and fixed the formatting nit for you, hopefully CI should pass. Thanks for all your work on this!

jysoo · 2019-06-14T18:00:43Z

@jysoo I rebased and fixed the formatting nit for you, hopefully CI should pass. Thanks for all your work on this!

Thank you!

…ngular#16018) * feat(datepicker): align multi-year-view based on minDate and maxDate If minDate is set (and maxDate not set), let first page begin with minYear. If maxDate is set, let last page end with maxYear, and disable year(s) < minYear (if any) in first page. Fixes angular#10646 * add tests for feat(datepicker): align multi-year-view based on minDate and maxDate * refactor feat(datepicker): align multi-year-view based on minDate and maxDate * omit helper func from public api * fixup, resolve conflict refactor with private methods feat(datepicker): align multi-year-view based on minDate and maxDate * share func feat(datepicker): align multi-year-view based on minDate and maxDate * fix formatting nit

…16018) * feat(datepicker): align multi-year-view based on minDate and maxDate If minDate is set (and maxDate not set), let first page begin with minYear. If maxDate is set, let last page end with maxYear, and disable year(s) < minYear (if any) in first page. Fixes #10646 * add tests for feat(datepicker): align multi-year-view based on minDate and maxDate * refactor feat(datepicker): align multi-year-view based on minDate and maxDate * omit helper func from public api * fixup, resolve conflict refactor with private methods feat(datepicker): align multi-year-view based on minDate and maxDate * share func feat(datepicker): align multi-year-view based on minDate and maxDate * fix formatting nit

angular-automatic-lock-bot · 2019-09-11T00:03:12Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jysoo requested a review from mmalerba as a code owner May 13, 2019 22:58

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 13, 2019

mmalerba reviewed May 14, 2019

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 17, 2019

mmalerba approved these changes May 17, 2019

View reviewed changes

Splaktar reviewed May 17, 2019

View reviewed changes

jelbourn reviewed May 18, 2019

View reviewed changes

jelbourn added target: patch This PR is targeted for the next patch release and removed action: merge The PR is ready for merge by the caretaker labels May 18, 2019

jelbourn reviewed May 18, 2019

View reviewed changes

Splaktar approved these changes May 19, 2019

View reviewed changes

src/material/datepicker/multi-year-view.ts Outdated Show resolved Hide resolved

jelbourn reviewed May 20, 2019

View reviewed changes

andrewseguin added the P4 A relatively minor issue that is not relevant to core functions label May 30, 2019

jysoo force-pushed the feature/datepicker-multi-year-view-min-max branch 3 times, most recently from 1c44b75 to 20c6256 Compare June 5, 2019 06:38

jelbourn reviewed Jun 5, 2019

View reviewed changes

jelbourn approved these changes Jun 14, 2019

View reviewed changes

jysoo and others added 7 commits June 14, 2019 10:23

feat(datepicker): align multi-year-view based on minDate and maxDate

7cb8795

If minDate is set (and maxDate not set), let first page begin with minYear. If maxDate is set, let last page end with maxYear, and disable year(s) < minYear (if any) in first page. Fixes angular#10646

add tests for feat(datepicker): align multi-year-view based on minDat…

449ede7

…e and maxDate

refactor feat(datepicker): align multi-year-view based on minDate and…

c9cd7ae

… maxDate

fixup, resolve conflict

85ebc49

refactor with private methods feat(datepicker): align multi-year-view based on minDate and maxDate

omit helper func from public api

9637949

share func feat(datepicker): align multi-year-view based on minDate a…

2480906

…nd maxDate

fix formatting nit

f708475

mmalerba force-pushed the feature/datepicker-multi-year-view-min-max branch from 9150ac0 to f708475 Compare June 14, 2019 17:29

mmalerba added the action: merge The PR is ready for merge by the caretaker label Jun 14, 2019

mmalerba merged commit f28a294 into angular:master Jun 17, 2019

jysoo deleted the feature/datepicker-multi-year-view-min-max branch June 20, 2019 19:30

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

feat(datepicker): align multi-year-view based on minDate and maxDate #16018

feat(datepicker): align multi-year-view based on minDate and maxDate #16018

Uh oh!

Conversation

jysoo commented May 13, 2019

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

jysoo commented May 17, 2019

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jysoo commented May 19, 2019

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jysoo commented Jun 14, 2019

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba commented Jun 14, 2019

Uh oh!

jysoo commented Jun 14, 2019

Uh oh!

angular-automatic-lock-bot bot commented Sep 11, 2019

Uh oh!

Uh oh!

Splaktar commented Jun 14, 2019 •

edited

Loading