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

Datepicker: months are 0-base indexed in NgbDate #728

Closed
pkozlowski-opensource opened this issue Sep 13, 2016 · 7 comments
Closed

Datepicker: months are 0-base indexed in NgbDate #728

pkozlowski-opensource opened this issue Sep 13, 2016 · 7 comments

Comments

@pkozlowski-opensource
Copy link
Member

While consistent with Date handling in JS I find it super-confusing. Let's discuss / re-consider.

Visible on our demo-page - select a data from a picker and you will see that the month is one behind.

@jnizet
Copy link
Member

jnizet commented Sep 13, 2016

Hah! I'm not alone :-) Java has finally abandoned 0-based months in its newer time API, so I'm used to having months 1-based (as they should be IMO). I wanted to submit the same remark, thinking moment.js at least did the right thing, but it doesn't either: 0-based months are used in moment, too.

So I guess it's a choice between being right, but inconsistent with everything else, and being wrong with all the other libraries :-)

I guess that when #545 is fixed and we have formatters/parsers, the issue will be less important because most people will probably bind to a string, a Date, or a moment, and the translation will be written once and for all in the formatter. So using 1-based months shouldn't annoy too many people then.

@pkozlowski-opensource
Copy link
Member Author

According to https://en.wikipedia.org/wiki/ISO_8601:

Calendar date representations are in the form shown in the box to the right. [YYYY] indicates a four-digit year, 0000 through 9999. [MM] indicates a two-digit month of the year, 01 through 12. [DD] indicates a two-digit day of that month, 01 through 31. For example, "5 April 1981" may be represented as either "1981-04-05"[5] in the extended format or "19810405" in the basic format.

As soon as we introduce formats / masks for dates it would be very awkward to keep using 0-based month numbers.

My vote would go to using 1-based indexes as we do for years and days. If there are no big objections I'm going to send a PR later in the week (or anyone is free to send one).

@maxokorokov
Copy link
Member

Agree with this one. The initial implementation was to be consistent with Javascript Date, but personally it feels weird to start with 0

@pkozlowski-opensource pkozlowski-opensource added this to the alpha.7 milestone Sep 23, 2016
@pkozlowski-opensource
Copy link
Member Author

Agree with this one. The initial implementation was to be consistent with Javascript Date, but personally it feels weird to start with 0

OK, let's do it then :-) @maxokorokov would you mind sending a PR for this somewhere next week? It would be one of breaking changes so I would like to get those in ASAP so we've got clear path to beta.0.

@maxokorokov maxokorokov self-assigned this Sep 23, 2016
@maxokorokov
Copy link
Member

FYI, I'm stick to the ISO numbering internally for default i18n as well:

1=Jan ... 12=Dec
1=Mon ... 7=Sun

For other calendars we'll see as they arrive

maxokorokov added a commit that referenced this issue Sep 26, 2016
BREAKING CHANGE: now datepicker uses ISO 8601 for month and weekday numbers with default calendar
Before
0=Jan; 1=Feb; ... 11=Dec
0=Sun; 1=Mon; ... 6=Sat

Now
1=Jan; 2=Feb; ... 12=Dec
1=Mon; 2=Tue; ... 7=Sun

Fixes #728 
Closes #797
@cguilhermef
Copy link

Men... This is a great mistake!

This plugin was made to be used in JavaScript, a 0-digit based. So, this plugin doesn't work with nothing: JavaScript native Date(), moment(), or any other thing build until today!
Bad choice.

@brian-griffin
Copy link

Men... This is a great mistake!

This plugin was made to be used in JavaScript, a 0-digit based. So, this plugin doesn't work with nothing: JavaScript native Date(), moment(), or any other thing build until today!
Bad choice.

I agree, this seems like an odd change to me. It says it's to follow the ISO 8601 format, but to me that's mixing data with formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants