feat(datepicker): add DateAdapter and NativeDateAdapter #4148

mmalerba · 2017-04-19T01:39:55Z

No description provided.

jelbourn · 2017-04-19T15:16:37Z

src/lib/core/datetime/date-adapter.ts

+   * @param date The date to extract the day of the week from.
+   * @returns The month component (0-indexed, 0 = Sunday).
+   */
+  abstract getDay(date: D): number;


Call this getDayOfTheWeek so that it can't possibly be mixed up with getDate?

jelbourn · 2017-04-19T15:19:53Z

src/lib/core/datetime/date-adapter.ts

+
+  /**
+   * Gets a list of names for the months.
+   * @param style The naming style (e.g. long = 'January', short = 'Jan', narrow = 'J').


style -> maybe format?

I called it style because I don't want people confusing it with the format: any that they would pass to the parse or format functions

jelbourn · 2017-04-19T15:24:34Z

src/lib/core/datetime/date-adapter.ts

+   * @param date The date of month of the date.
+   * @returns The new date.
+   */
+  abstract create(year: number, month: number, date: number): D;


Make this createDate so that in the future we can add createTime and createDatetime?

jelbourn · 2017-04-19T15:25:35Z

src/lib/core/datetime/date-adapter.ts

+   * @param locale The locale of the value.
+   * @returns The parsed date, or null if date could not be parsed.
+   */
+  abstract parse(value: any, fmt?: any, locale?: any): D | null;


Should value not always be a string?

Can you elaborate more on what the format is meant to be here?

jelbourn · 2017-04-19T15:27:24Z

src/lib/core/datetime/date-adapter.ts

+   * @param amount The number of years, months, and days to add (may be negative).
+   * @returns A new date equal to the original with the given amount of time added.
+   */
+  abstract add(date: D, amount: {years?: number, months?: number, days?: number}): D;


Related: https://momentjs.com/guides/#/lib-concepts/date-time-math/

amount -> timespan?

It's worth thinking about whether we want to separate the logic for adding "date spans" (years, months, days) versus "time spans" (maybe be hours, minutes, seconds, but eventually boils down to milliseconds). If not, then the single add could surprise people because the order of operations will affect the outcome. E.g., adding "1 month" will be a variable number of days based on where you are in the year. If it's broken up, then this method should probably be something like addDateSpan with addTimeSpan being added later.

Hmm interesting... I assumed moment added years first then months then days, like how I do for Date, but it seems it does the opposite order. Do you think I should add largest unit first, smallest unit first, or only allow adding 1 unit at a time and force the user to determine the order?

Only adding one unit at a time is probably the only way to ensure that it does exactly what the user expects (e.g., addYears, addMonths, etc.). If we assume any kind of order of operations, I'd want to run a quick question by Google's i18n experts as to how they would say it should behave.

I'm also noticing that moment and closure seem to add what I'm calling 'calendarDays', 'calendarMonths', and 'calendarYears' here: https://github.com/angular/material2/blob/datepicker/src/lib/datepicker/calendar.ts#L271 I think I'll make add do that to be more in line with these other libraries

jelbourn · 2017-04-19T15:47:07Z

src/lib/core/datetime/native-date-adapter.ts

+}
+
+
+/** Adapts the native JS Date for use with material components that work with dates. */


Don't mention material specifically here since this will go in the cdk.

jelbourn · 2017-04-19T15:56:19Z

src/lib/core/datetime/native-date-adapter.ts

+  }
+
+  getMonthNames(style: 'long' | 'short' | 'narrow', locale?: any): string[] {
+    let dtf = new Intl.DateTimeFormat(locale, {month: style});


Since NativeDateAdapter will be the default if the user does nothing, we still have to support Intl not being available for Safari 9 (...probably? We should ask devrel for an opinion on this, since omitting the strings would be nice for payload size).

jelbourn · 2017-04-19T16:05:36Z

src/lib/core/datetime/date-adapter.ts

+   * @returns {boolean} Whether the two dates are equal.
+   *     Null dates are considered equal to other null dates.
+   */
+  equals(first: D | null, second: D | null): boolean {


Similar comment as above for equals and compare: you'll want different things for the time variants. Maybe just call them compareDate and sameDate?

jelbourn · 2017-04-19T16:07:50Z

src/lib/core/datetime/native-date-adapter.spec.ts

+    expect(adapter.create(50, -12 * 51, 1).getFullYear()).toBe(-1);
+  });
+
+  it('should get today\'s date', () => {


optional: you can use double quotes (or template string backticks) when it's to avoid escaping single quotes

jelbourn · 2017-04-19T16:08:50Z

src/lib/core/datetime/date-adapter.ts

+   * @returns `min` if `date` is less than `min`, `max` if date is greater than `max`,
+   *     otherwise `date`.
+   */
+  clamp(date: D, min?: D | null, max?: D | null): D {


Same thing about date vs. time/datetime

mmalerba · 2017-04-19T23:40:45Z

comments addressed, PTAL

jelbourn · 2017-04-20T16:10:01Z

src/lib/core/datetime/date-adapter.ts

+   * @param fmt The format to use for the result string.
+   * @returns The parsed date, or null if date could not be parsed.
+   */
+  abstract format(date: D, fmt?: any): string;


With this one format method, are we going to be able to get the different formats we need for date / time / datetime?

I think this should be ok Intl.DateTimeFormat, moment.js, and goog.date.Date all support this

jelbourn · 2017-04-20T16:11:08Z

src/lib/core/datetime/date-adapter.ts

+   * @returns A new date equal to the given date.
+   */
+  clone(date: D): D {
+    return this.createDate(this.getYear(date), this.getMonth(date), this.getDate(date));


Would it be better to leave this without an implementation here so that the method can be used for all of date/time/datetime?

jelbourn · 2017-04-20T16:13:45Z

src/lib/core/datetime/native-date-adapter.ts

+        'October', 'November', 'December'
+      ],
+      'short': ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec'],
+      'narrow': ['J', 'F', 'M', 'A', 'M', 'J', 'J', 'A', 'S', 'O', 'N', 'D']


By hard-coding the values into the return, you'll end up creating a new instance of these arrays each time the method is called. Move this object into a constant?

jelbourn · 2017-04-20T16:23:03Z

src/lib/core/datetime/native-date-adapter.spec.ts

@@ -0,0 +1,242 @@
+import {NativeDateAdapter} from './native-date-adapter';
+
+


In the material1 tests, I used constants for month indices to make the tests easier to read:

// When constructing a Date, the month is zero-based. This can be confusing, since people are // used to seeing them one-based. So we create these aliases to make reading the tests easier. var JAN = 0, FEB = 1, MAR = 2, APR = 3, MAY = 4, JUN = 5, JUL = 6, AUG = 7, SEP = 8, OCT = 9, NOV = 10, DEC = 11;

Then your tests look like

expect(adapter.getYear(new Date(2017, JAN, 1))).toBe(2017);

Make a similar change here?

mmalerba · 2017-04-20T17:26:50Z

comments addressed

jelbourn

LGTM

* added DateAdapter and NativeDateAdapter * add tests * addressed some comments * more comments addressed * another round of comments addressed * add default formats

angular-automatic-lock-bot · 2019-09-06T11:03:06Z

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.}

mmalerba added the pr: needs review label Apr 19, 2017

mmalerba assigned jelbourn Apr 19, 2017

mmalerba requested a review from jelbourn April 19, 2017 01:39

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 19, 2017

mmalerba force-pushed the dp-date branch from 4884c9b to 984d66f Compare April 19, 2017 15:53

mmalerba added 2 commits April 19, 2017 08:53

added DateAdapter and NativeDateAdapter

347fb88

add tests

86fea16

mmalerba force-pushed the dp-date branch from 984d66f to 86fea16 Compare April 19, 2017 15:53

jelbourn reviewed Apr 19, 2017

View reviewed changes

addressed some comments

ded42f4

mmalerba force-pushed the dp-date branch 2 times, most recently from 00c94de to d5aa3f3 Compare April 19, 2017 23:36

more comments addressed

3621d43

mmalerba force-pushed the dp-date branch from d5aa3f3 to 3621d43 Compare April 19, 2017 23:39

jelbourn reviewed Apr 20, 2017

View reviewed changes

mmalerba added 2 commits April 20, 2017 10:12

another round of comments addressed

a96880c

add default formats

8ef4362

jelbourn approved these changes Apr 20, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 20, 2017

mmalerba merged commit 52400df into angular:datepicker Apr 20, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): add DateAdapter and NativeDateAdapter #4148

feat(datepicker): add DateAdapter and NativeDateAdapter #4148

mmalerba commented Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

mmalerba Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

mmalerba Apr 19, 2017

jelbourn Apr 19, 2017

mmalerba Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

jelbourn Apr 19, 2017

mmalerba commented Apr 19, 2017

jelbourn Apr 20, 2017

mmalerba Apr 20, 2017

jelbourn Apr 20, 2017

jelbourn Apr 20, 2017

jelbourn Apr 20, 2017

mmalerba commented Apr 20, 2017

jelbourn left a comment

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

		}


		/** Adapts the native JS Date for use with material components that work with dates. */

		@@ -0,0 +1,242 @@
		import {NativeDateAdapter} from './native-date-adapter';

feat(datepicker): add DateAdapter and NativeDateAdapter #4148

feat(datepicker): add DateAdapter and NativeDateAdapter #4148

Conversation

mmalerba commented Apr 19, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Apr 19, 2017

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

mmalerba commented Apr 20, 2017

jelbourn left a comment

Choose a reason for hiding this comment

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