Use moment locales for customization

[rafeememon]

Update of #257, #228; fixes #175, #178; probably fixes #126, #160, #203

This is a radical change of how we handle locales. Instead of giving customization points to effectively change the date picker's locale (namely, locale, weekdays, and weekStart), this would completely defer to moment's handling of locales for custom weekdays/months/etc. I think this is reasonable since applications with different locales will most likely in practice be configuring the moment locale anyway, and if not it's reasonable to request them to do so. After my findings for #257, this would be the simplest thing to do going forward from both a component API and maintainability standpoint. But obviously since this is an API break it needs to be carefully considered.

Thoughts/comments welcome!

[martijnrusschen]

Interesting, if we do this, how would the API for handling locales, custom week days look like? We should make sure the examples are updated/added too. (since this is a breaking change) For me, this sound like the biggest blocker before we move to a v1.0 of this component.

[martijnrusschen]

Also, adding @RSO here to discuss the change and the future of using moment.js in the component.

[rafeememon]

We would depend on the consumer to customize the moment locale themselves -- it would actually be very similar to what happens in the initializeMomentLocale method that is removed in this PR. To have the same effect they would need to do this (where locale, weekStart, and weekdays are the old properties):

import moment from 'moment';

...

moment.locale(locale, {
  week: {
    dow: weekStart
  },
  weekdaysMin: weekdays
});

This would also open up the ability to customize other things, like month names. Once we have the complete story figured out I'll update the docs and examples with our suggestions.

I was thinking this could be part of a v1.0 milestone as well. I'll try to think of other things that should be included in the major release.

[martijnrusschen]

Yep, as long as it stays easy to implement and use this component. Are we able to set some sane default s of the moment locales? Would prefer having a good default setup that people can overwrite if they want.

[JeffGuKang]

I think it will better to remain customization points including month such as weekdays, months and weekStart for various use.
And setting the locale by a country code will be the best way, in my case.

<DatePicker
  locale="ko"
  selected={this.state.endDate}
  onChange={this.handleChangeDate}
/>

Now I am applying a localization by import from moment

var moment = require('moment');
require('moment/locale/ko');

<DatePicker
 locale="ko" 
 weekdays={['일', '월', '화', '수', '목', '금', '토']}
 weekStart='1'
 moment={moment}
 selected={this.state.endDate}
 onChange={this.handleChangeDate}
/>

Actually, I had not known how I could do localization until I found docs from moment.js

[martijnrusschen]

Sounds interesting. @rafeememon What do you think?

[rafeememon]

I think in @JeffGuKang's case, he was being limited by the default props, specifically locale: "en" and weekdays: ["Su", "Mo", "Tu", "We", "Th", "Fr", "Sa"], which is why he is forced to pass in values to override them. Ideally the datepicker should be able to work easily when the global moment locale is set, e.g. with require('moment/locale/ko').

[rafeememon]

Found some more extreme sadness re: locales

• Setting locales locally on moment objects only allows setting the language; options like weekdays and week start are not available http://momentjs.com/docs/#/i18n/instance-locale/
• If the weekdays and week start options are set globally, the change propagates to the configuration of the entire locale, like en. It must be the case that locale info is stored in one central place in moment.js and is not copied when configured manually

rafee = moment().locale('en')
rafee.localeData().firstDayOfWeek() // 0
rafee.clone().startOf('week').toISOString() // 2016-02-07T00:00:00.000Z
moment.locale('en', {week:{dow:4}})
rafee.localeData().firstDayOfWeek() // 4
rafee.clone().startOf('week').toISOString() // 2016-02-11T00:00:00.000Z

Mainly because of the second point, I think the weekdays and weekStart options are _unsupportable_ if we don't want to modify the global state. However, because of the first point, we can still support the locale prop, and defer to the global config if it's not set (by removing the default locale prop).

[rafeememon]

Updated my PR to reflect my new vision. Thoughts/comments welcome from everyone!

[rafeememon]

Localizing the moment as soon as it's passed in allows the localization to be passed to weekdays and week start automatically.

[rafeememon]

Hooray for tests!

[codecov-io]

Current coverage is 93.61%

Merging #277 into master will increase coverage by +0.98% as of 578f97e

@@            master    #277   diff @@
======================================
  Files            9       9       
  Stmts          190     188     -2
  Branches        37      38     +1
  Methods         90      88     -2
======================================
  Hit            176     176       
+ Partial          4       3     -1
+ Missed          10       9     -1

Review entire Coverage Diff as of 578f97e

Powered by Codecov. Updated on successful CI builds.

[rafeememon]

After sleeping on it I've become increasingly convinced that this is the right way to go. I've updated the readme and examples with the new approach. @martijnrusschen, @RSO, could you take another look at this?

[martijnrusschen]

I like the approach. Especially the fact that you can use your global moment locale in the component. Furthermore, the ability to pass in a moment locale as a prop makes it behaving the same as before.

[rafeememon]

Let's do it -- these fixes are a long time coming

[martijnrusschen]

Nice!

[JeffGuKang]

👍