search

moment / moment

Parse, validate, manipulate, and display dates in javascript.
momentjs.com
MIT License
47.99k stars 7.05k forks source link

Incorrect first day of week for Indonesian locale #5370

Closed kstinson14 closed 4 years ago

kstinson14 commented 4 years ago

Currently, the Indonesian locale has Monday listed as the first calendar day of the week. My i10n team has said that this is incorrect, and Sunday should be the first day instead.

Searching for the correct answer gave me some mixed results, but I found that the Unicode Common Locale Data Repository supports the case for using Sunday instead of Monday. Windows also follows this configuration. When selecting Indonesian as your locale on Windows 10, Sunday is the default option for first day of the week.

Screen Shot 2020-02-03 at 10 58 12 AM

I could submit a PR to change this if desired.

@tyok

tyok commented 4 years ago

Both can be used as the first day. In the popular children's song "nama-nama hari" it is Monday while in the official dictionary it is Sunday (https://kbbi.kemdikbud.go.id/entri/Minggu).

I think Sunday-first is more popular in the digital context, since that's the behavior in Windows and Android Calendar.

kstinson14 commented 4 years ago

Ah, ok. Do you think it would make sense to change here then?

tyok commented 4 years ago

Sorry for not being clear, yeah I think it's ok to change it.

mantinone commented 4 years ago

Hi, I'm trying to get into doing regular open source contributions and am looking for issues to pick up. May I claim this issue?

kstinson14 commented 4 years ago

@mantinone Sounds good, I'll hold off on PR if you'd like to grab this one 👍

mantinone commented 4 years ago

@kstinson14 Thank you!

I thought this would be a pretty trivial change, but there's more to this than I thought, and I've got questions about what changes need to be made. (and maybe I should tag @tyok here?) It seems that the doy (the day in January that marks the first week of the year) needs to be changed as well as the first day of the week, and I'm not sure what it should be changed to and what the tests should look like.

The example failing test Grunt gives me is 

weeks year starting sunday formatted Jan  8 2012 should be week 2
...
Actual value:
3 03 3
Expected value:
2 02 2

Previously we had dow: 1 //Monday and doy: 7 //week containing January 7th. I did a search for all locales with dow: 0 //Sunday and found that none of them have doy: 7.
doy: 6 is fairly common, though. So should id/js be changed to Jan 6th as well?

kstinson14 commented 4 years ago

@mantinone Sorry for the delayed response.

Yeah, I think this is a @tyok question. I'm not sure what decides the doy value in this locale. (here is an issue talking a bit about how it's determined) but I think he would know.

mantinone commented 4 years ago

Perhaps it's a good time to ping @tyok again?

Or maybe another contributor, since this seems like a more general question about how moment defines First Week of the year. I'm not sure who to tag, though. @timrwood maybe?

mantinone commented 4 years ago

Does anyone know who would be a good person to ask about this question, or where there's documentation talking about it?

Should I just make a guess and do a pull request? The changes are fairly straightforward. I just don't know precisely what changes to make.

marwahaha commented 4 years ago

Thanks for offering to make this change.

Need to change src/locale/id.js and associated tests.

Docs for this change are at https://momentjs.com/docs/#/customization/dow-doy/

Probably, (dow, doy) go to (0, 6) based on formula listed in docs.

You can ping me if you need help.

mantinone commented 4 years ago

Thank you for pointing out the part in the docs that I missed that explains this!
The formula is still confusing to me because I'm not sure how they're determining what janX is.

I went ahead and submitted the PR with doy as 6.