feat(datepicker): Add Moment.js adapter #6860

mmalerba · 2017-09-05T22:31:37Z

No description provided.

mmalerba · 2017-09-05T22:35:23Z

There's still something in the build configuration that isn't right. We get an error TypeError: moment_1.default is not a function I believe this is a result of the synthetic default import...

jelbourn · 2017-09-05T22:52:33Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+    this.setLocale(dateLocale || moment.locale());
+  }
+
+  setLocale(locale: any) {


Can the type be more specific than any here since you know you're working with whatever moment supports?

jelbourn · 2017-09-05T22:58:03Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+
+    // Temporarily change the global locale to get the data we need, then change it back.
+    let globalLocale = moment.locale();
+    moment.locale(locale);


Instead of changing the locale, can't you pass an argument to moment.localeData()?

const momentLocaleData = moment.localeData(locale); this._localeData = { firstDayOfWeek: momentLocaleData.firstDayOfWeek(), longMonths: momentLocaleData.months(), ... };

Somehow missed the fact that you can do that

jelbourn · 2017-09-05T22:59:17Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+export class MomentDateAdapter extends DateAdapter<moment.Moment> {
+  // Note: all of the methods that accept a `moment.Moment` input parameter immediately call
+  // `this.clone` on it. This is to ensure that we're working with a `moment.Moment` that has the
+  // correct locale setting while avoiding mutating the original object passed to us.


Why is this necessary for things that read and return a primitive, e.g. getYear? How would the instance be mutated there? Might be helpful to include an example of what you're trying to avoid

jelbourn · 2017-09-05T23:00:57Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+    moment.locale(globalLocale);
+  }
+
+  getYear(date: moment.Moment): number {


Make an alias at the beginning of the file for brevity?

type Moment = moment.Moment;

jelbourn · 2017-09-05T23:06:00Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+
+  createDate(year: number, month: number, date: number): moment.Moment {
+    // Check for invalid month and date (except upper bound on date which we have to check after
+    // creating the Date).


Mention what moment would otherwise do if the month is out of range?

Moment actually does the right thing, just want to be able to give better errors to the user

jelbourn · 2017-09-05T23:07:59Z

src/material-moment-adapter/adapter/moment-date-adapter.spec.ts

+// Add some locales for testing. These definitions come from Moment.js's fr.js and ja.js locale
+// files. (We don't want to the version of moment that comes with locales because it's a lot of
+// extra bytes to include in our tests.)
+moment.defineLocale('fr', {


Move this constant data into a separate file and import it?

mmalerba · 2017-09-07T23:54:33Z

Comments addressed, PTAL. Super huge thank you to @devversion for helping with a bunch of build issues

jelbourn

LGTM, just one more request for comment

jelbourn · 2017-09-08T16:34:58Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

+import {DateAdapter, MAT_DATE_LOCALE} from '@angular/material';
+
+// Depending on whether rollup is used, moment needs to be imported differently.
+// TODO(mmalerba): See if we can clean this up at some point.


Can you expand this comment to explain more of what's happening?

devversion

Sorry for the trouble with the build system. Bazel 🔜

devversion · 2017-09-08T18:22:06Z

tsconfig.json

@@ -2,6 +2,8 @@
 // IDEs and TSLint. For IDEs it ensures that `experimentalDecorator` warnings are not showing up.
 {
  "compilerOptions": {
+    // Needed for Moment.js since it doesn't have a default export.
+    "allowSyntheticDefaultImports": true,


I don't think we need that anymore right?

We need it if we want webstorm to not complain about import {default as moment} from 'moment', not sure if we care about that or not...

christele15 · 2017-09-11T15:36:18Z

Good morning,
I am a newbie to Angular, and I am sorry if that might be a newbie question but I was tasked to use the datepicker and trigger an event if the current date selected is less than 27 weeks, more than 27 weeks or 27 weeks, is this something that might be doable, and is there anything to guide me ?

back as a docs example in future PR).

This reverts commit 9545427.

* Revert "feat(datepicker): Add Moment.js adapter (#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (#6961)" This reverts commit 1cccd4b.

This reverts commit 9545427.

* Revert "Revert "fix(menu): multiple close events for a single close" (#7036)" This reverts commit dcfe515. * Revert "feat(datepicker): Add Moment.js adapter (#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (#6961)" This reverts commit 1cccd4b. * Revert "fix(menu): nested trigger staying highlighted after click (#6853)" This reverts commit 04bf3d1. * Revert "feat(viewport-ruler): add common window resize handler (#6680)" This reverts commit 881630f.

another-guy · 2017-09-14T14:49:33Z

While having a moment.js based date adapter is better than nothing, there are still little questions after reviewing the code.

Was date-fns considered as an immutable alternative? Using it would help reducing the potential defect count, and would get rid of the necessity to manually this.clone(...) the date in every method.

My related post and a comment on the related subject got zero attention, even though I provided a code snippet with ISO8601 date adapter that is the first step to a pull request.

mmalerba · 2017-09-14T16:14:17Z

@another-guy I'd certainly be open to adding a date-fns one as well, but last I checked it didn't support custom parsing formats which is the main thing lacking in the NativeDateAdapter, that's why I prioritized adding the MomentDateAdapter. We also expect users to create their own DateAdapters if they want something a little different than what we offer by default.

another-guy · 2017-09-14T17:15:00Z

@mmalerba Good to know. I was sure there is a strong reason to use moment.js over the date-fns. Thanks for the answer!

* create moment adapter * add Moment to karma.conf.js * use MAT_DATE_LOCALE * tests are now running * add in tests * WIP: demo * add synthetic default imports for moment * add locale switching to demo * fix moment import issues with tests * fix aot * address comments * fix closure issue * add additional explanation of rollup issue * remove moment adapter demo since it is preventing sync into g3 (will add back as a docs example in future PR). * fix bad rebase

…#7036) * Revert "feat(datepicker): Add Moment.js adapter (angular#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (angular#6961)" This reverts commit 1cccd4b.

…lar#7054) * Revert "Revert "fix(menu): multiple close events for a single close" (angular#7036)" This reverts commit dcfe515. * Revert "feat(datepicker): Add Moment.js adapter (angular#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (angular#6961)" This reverts commit 1cccd4b. * Revert "fix(menu): nested trigger staying highlighted after click (angular#6853)" This reverts commit 04bf3d1. * Revert "feat(viewport-ruler): add common window resize handler (angular#6680)" This reverts commit 881630f.

andlcool · 2017-09-28T05:57:26Z

Thanks for the moment date adapter. This will come in handy.

Are there examples on how to implement this? Thanks.

angular-automatic-lock-bot · 2019-09-08T16:03:01Z

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 Sep 5, 2017

mmalerba assigned jelbourn and devversion Sep 5, 2017

mmalerba requested review from jelbourn and devversion September 5, 2017 22:31

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

mmalerba force-pushed the date-adapter branch from 762a341 to dba2aad Compare September 5, 2017 22:34

jelbourn reviewed Sep 5, 2017

View reviewed changes

mmalerba mentioned this pull request Sep 7, 2017

datepicker doesn't open to correct month for non US date format #5665

Closed

mmalerba force-pushed the date-adapter branch 2 times, most recently from 0c59d70 to cbeb48e Compare September 7, 2017 20:12

mmalerba force-pushed the date-adapter branch 4 times, most recently from 5d39384 to a0e60b3 Compare September 8, 2017 16:34

jelbourn approved these changes Sep 8, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Sep 8, 2017

mmalerba force-pushed the date-adapter branch from a0e60b3 to db1514d Compare September 8, 2017 17:06

devversion approved these changes Sep 8, 2017

View reviewed changes

devversion added the action: merge The PR is ready for merge by the caretaker label Sep 11, 2017

mmalerba force-pushed the date-adapter branch 2 times, most recently from 8e626c9 to f46106f Compare September 12, 2017 18:50

mmalerba added 4 commits September 12, 2017 13:06

create moment adapter

82b802b

add Moment to karma.conf.js

dedf235

use MAT_DATE_LOCALE

3a6a0cf

tests are now running

f39e9d8

mmalerba added 10 commits September 12, 2017 13:06

add in tests

a81eeea

WIP: demo

5aedf7e

add synthetic default imports for moment

d10ec97

add locale switching to demo

9fd85e4

fix moment import issues with tests

c1d23ac

fix aot

5c7ba67

address comments

add6ff0

fix closure issue

ef10910

add additional explanation of rollup issue

e68ab35

remove moment adapter demo since it is preventing sync into g3 (will add

bbdb0f9

back as a docs example in future PR).

mmalerba force-pushed the date-adapter branch from f46106f to bbdb0f9 Compare September 12, 2017 20:06

fix bad rebase

be25474

mmalerba merged commit 9545427 into angular:master Sep 12, 2017

mmalerba added a commit that referenced this pull request Sep 13, 2017

Revert "feat(datepicker): Add Moment.js adapter (#6860)"

75dc9e6

This reverts commit 9545427.

mmalerba added a commit that referenced this pull request Sep 13, 2017

Revert "feat(datepicker): Add Moment.js adapter (#6860)"

4bddcee

This reverts commit 9545427.

emreavsar mentioned this pull request Oct 2, 2017

Add documentation for MomentDateAdapter usage #7464

Closed

mmalerba deleted the date-adapter branch April 3, 2018 15:19

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): Add Moment.js adapter #6860

feat(datepicker): Add Moment.js adapter #6860

mmalerba commented Sep 5, 2017

mmalerba commented Sep 5, 2017

jelbourn Sep 5, 2017

jelbourn Sep 5, 2017

mmalerba Sep 7, 2017

jelbourn Sep 5, 2017

jelbourn Sep 5, 2017

jelbourn Sep 5, 2017

mmalerba Sep 7, 2017

jelbourn Sep 5, 2017

mmalerba commented Sep 7, 2017

jelbourn left a comment

jelbourn Sep 8, 2017

devversion left a comment

devversion Sep 8, 2017

mmalerba Sep 8, 2017

christele15 commented Sep 11, 2017

another-guy commented Sep 14, 2017

mmalerba commented Sep 14, 2017

another-guy commented Sep 14, 2017

andlcool commented Sep 28, 2017

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

feat(datepicker): Add Moment.js adapter #6860

feat(datepicker): Add Moment.js adapter #6860

Conversation

mmalerba commented Sep 5, 2017

mmalerba commented Sep 5, 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

mmalerba commented Sep 7, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christele15 commented Sep 11, 2017

another-guy commented Sep 14, 2017

mmalerba commented Sep 14, 2017

another-guy commented Sep 14, 2017

andlcool commented Sep 28, 2017

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