Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(datepicker): add aditional DateAdapters #5972

Closed
mmalerba opened this issue Jul 22, 2017 · 24 comments
Closed

feat(datepicker): add aditional DateAdapters #5972

mmalerba opened this issue Jul 22, 2017 · 24 comments
Assignees
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix

Comments

@mmalerba
Copy link
Contributor

Add additional DateAdapters that support libraries like Moment.js and date-fns

@mmalerba mmalerba added the feature This issue represents a new feature or feature request rather than a bug or bug fix label Jul 22, 2017
@mmalerba mmalerba self-assigned this Jul 22, 2017
@alexciesielski
Copy link

alexciesielski commented Aug 3, 2017

I have created a working date adapter for use with MomentJS (german locale only) for a personal project.
If interested, see https://gist.github.com/ciesielskico/f808e5f26324e8d3e494ac7a52ba6749

@peymanebrahimi
Copy link

here is an adapter for jalali calendar.
based on angular material beta8.
https://github.com/peymanebrahimi/AngularMaterialJalaliDate
date system which is used in Iran.

@mmalerba
Copy link
Contributor Author

mmalerba commented Aug 28, 2017

@peymanebrahimi very cool! Have you found that the DateAdapter is flexible enough to easily create adapters for other calendar systems? I ask because I know the datepicker does make a few assumptions (e.g. that weeks are 7 days long) and I'm curious if these are safe assumptions to make, or if they're actually things that might change in other calendar systems

@peymanebrahimi
Copy link

It's flexible enough.
I am not aware of other official calendars of other countries for assumptions like number of days in a week. but I ll do a search and let you know.
There are some behavior of the adapter or datepicker itself:

  1. The jalali plugin for momentjs returns the day of week correctly but inside my adapter I should subtract 1 inside getDayOfWeek() to adjust name of days in a month in datepicker.
  2. All the inputs of the adapter functions which should be of type of T of determined generic, are not type of T and are javascript Date.

@mmalerba
Copy link
Contributor Author

Ah that's awesome to hear, glad that it wasn't too hard to get working! :)

  1. Yes, some date implementation use 0-indexed day of the week and others use 1-indexed. I went with 0-indexed in the date adapter so 1-indexed implementations will have to subtract
  2. Are all of the inputs that you're passing to the md-datepicker actually Moment objects? If you're passing strings or native Date objects that may explain why the wrong type is coming through to the adapter.
startDate = jmoment('2017-01-01', 'YYYY-MM-DD');
// rather than:
// startDate = '2017-01-01';
<md-datepicker [startAt]="startDate"></md-datepicker>

@peymanebrahimi
Copy link

peymanebrahimi commented Aug 30, 2017

Thanks.
Ok, lets consider simple datepicker without startAt;
inside adapter I still get a js Date not a moment Date.
image

About item 1, I think jalali-moment uses zero-indexed day of week.
For Iranian calendar, inside getFirstDayOfWeek() we return 6. which is Saturday.
and datepicker correctly shows the narrow form of Saturday in Persian as first item of the row of week names.
and for a Wednesday, the library returns 4 for getDayOfWeek ; without subtracting 1; which seems to be correct by itself. But then days in the month are wrong as columns with Day Name header. (as mentioned in my previous comment).

@mmalerba
Copy link
Contributor Author

mmalerba commented Sep 1, 2017

@peymanebrahimi Ah I might know what's going on for the first issue. Moment.js has 2 ways to get the day .day() and .weekday() (https://momentjs.com/docs/#/get-set/day/) .weekday() is locale-aware meaning it will report Saturday as 0 if your locale is persian. I think in order to fix this you can either switch to using .day() or change your getFirstDayOfWeek() method to return 0 since the shift is already accounted for by moment's weekday function. Perhaps this is a flaw in the API, it's a little confusing when there's multiple ways to do it...

I'm not completely sure why you're seeing non-Moment objects come through, that seems strange. I'm working on a Moment.js adapter right now, so I'll either run into the same problem and fix the bug or if I don't see the issue, we can compare implementations and see what's different. (My adapter is just for normal Moment, not Jalali Moment, so thanks for putting this out there!)

@peymanebrahimi
Copy link

very good
.day() instead of .weekday() works fine. (no need for subtracting 1).
I should not change the getFirstDayOfWeek to 0, since it alters the sequence of name of weekdays in the datepicker and it does not make sense for us having a week started in Sunday.
For now the type of input parameter considered as pure js Date since we pass the input to the moment constructor as:

 getYear(date: jmoment.Moment): number { 
        return jmoment(date).jYear();
        //instead of  return date.jYear();
    }

My code on github.
sorry if it is not on plunkr and https://stackblitz.com/ is still in beta.

startDate1 =  jmoment('2017-01-01', 'YYYY-MM-DD');
startDate2 = jmoment('1395-10-12', 'jYYYY-jMM-jDD');

was a good point. Thanks.
But this alter the name of month besides the year inside the button that toggles the year or month view but month name is correct beside days and in year view, all month names are correct.
Which may be a matter of MD_DATE_FORMATS.
image

AND
whether startDate is string or jmoment, in datepicker, we does not have a circle around the day but it goes to correct month.

@peymanebrahimi
Copy link

Hi @mmalerba
Thanks for improvement.
Do you really have moment object here ?

  clone(date: Moment): Moment {
    return date.clone().locale(this.locale);
  }

I updated for

all of the methods that accept a Moment input parameter immediately call this.clone

but the clone itself is:
return jmoment(date).clone().locale(this.locale);
because otherwise I get this error:
ERROR TypeError: date.clone is not a function
debugging info is exact as before.

@mmalerba
Copy link
Contributor Author

For me in both my tests and the demo application I only ever got Moment objects passed in. Perhaps you could try copying the relevant tests I created for mine here: https://github.com/angular/material2/blob/master/src/material-moment-adapter/adapter/moment-date-adapter.spec.ts and see if they catch any issues. I'm not sure how non-Moment objects are getting passed in to your code...

@peymanebrahimi
Copy link

Ok, i ll check. tnx

@peymanebrahimi
Copy link

In the middle of writing test:
I encountered a problem which inside my adapter the constructor is called twice.
First time with the dateLocale => "fa", and second time with "en-US" .
There is only one component which news up the adapter.
this.adapter = new JalaliMomentDateAdapter('fa'); and this call is inside component constructor.
In Call Stack for second call we have _createClass core.es5.js:9520.
I am using "@angular/material": "github:angular/material2-builds".

And when moving adapter newing to ngOnInit, the order of constructor calls reverses.
By the way inside methods of adapter which need .locale(this.locale) this .locale is en-US

clone(date: jmoment.Moment): jmoment.Moment {
        console.log('this.locale inside clone: ', this.locale); // en-US
        return jmoment(date).clone().locale(this.locale);
    }

@peymanebrahimi
Copy link

What is the whole purpose of the ability of changing locale in the adapter?

When passing 3 numbers as year, month, day to createDate() , they are considered as Jalali system numbers therefore I can not simply pass them to moment constructor. The implementation is deferent.
Then changing locale solely from "fa" to "en", canot result in correct calculation.

If somebody needs to have Gregorian datepicker and also Persian datepicker, then she must have multiple adapter.

But
But for all who use one calendar system e.g. US and Germany and only the formatting and naming is different then the ability to change the locale in adapter is very useful and no need to have multiple adapters.

Am I on the right track?

Thanks for your time and effort.

@mmalerba
Copy link
Contributor Author

The double construction issue is probably because the dependency injection system is constructing an instance automatically using the MAT_DATE_LOCALE as the value for the locale. I think for your adapter you can probably not bother injecting this in the constructor and just set the locale to 'fa' since that's the only valid value. Or you could set up your module to provide 'fa' as the value for MAT_DATE_LOCALE.

And yes, setting the locale is useful since there are many locales that use the Gregorian calendar, but it doesn't really make sense for this adapter.

@peymanebrahimi
Copy link

Ok, Thanks.
Works almost prefect.
All passed in parameters are typeof moment.
MAT_DATE_LOCALE in constructor ignored. All .locale() hard coded to 'fa'
Code updated.
On a fresh page, on datepicker popup, for the very first time , day narrow names are in English!
after closing and reopening they are in Persian.
Is there any assumption on locale = en on view construction? Or any mistake on my side?

Really Appreciated

@mmalerba
Copy link
Contributor Author

hmm strange, perhaps jmoment's global locale is set to 'en' initially?

you could try changing:

return jmoment().localeData().weekdaysMin().slice(0);

to:

return jmoment().localeData('fa').weekdaysMin().slice(0);

and see if that helps.

@peymanebrahimi
Copy link

Thanks man.
That was it.
Now works fine.
Thank you very much.

@mmalerba
Copy link
Contributor Author

Moment.js adapter has been added, date-fns doesn't support parsing formats yet which would be the main reason to make a custom adapter for it. Therefore closing this issue

@rafaelss95
Copy link
Contributor

@mmalerba can you please show a basic use of MomentAdapter?

@mmalerba
Copy link
Contributor Author

I'm working on updating the docs right now, but just import {MatMomentDateModule} from '@angular/material-moment-adapter' and use it in place of MatNativeDateModule. I don't think its published to npm just yet, but you can get it here: https://github.com/angular/material2-moment-adapter-builds

@jbojcic1
Copy link
Contributor

@mmalerba when can we expect it in npm?

@jpduckwo
Copy link

Hanging out for @angular/material-moment-adapter in npm :)

@jpduckwo
Copy link

Got it working with "@angular/material": "^2.0.0-beta.12"
using:

npm install github:angular/material2-moment-adapter-builds --save

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix
Projects
None yet
Development

No branches or pull requests

6 participants