feat(calendar): add firstDayOfWeek and days abbreviations #2899

balthazar · 2016-01-12T17:32:25Z

This commit adds the ability to set the firstDayOfWeek for Europeans that often starts on monday rather than sunday, and to set a days abbreviations array for days first letter.

This part could be improved though, but don't really know how, feel free to tell me if you got an idea.

Related to #2099.

oliviertassinari · 2016-01-17T10:01:41Z

src/date-picker/date-picker.jsx

+     * Used to change the first day of week
+     * Defaults to 0 (Sunday)
+     */
+    firstDayOfWeek: React.PropTypes.number,


Can't we use Intl to do this work for us?
This can help #2282

balthazar · 2016-01-17T12:06:56Z

@oliviertassinari Thanks, I will update my PR and ping again

balthazar · 2016-01-17T12:13:12Z

I though it would be a little overkill to create 7 dates only to find a char, don't we have any way to access the Intl locales without having to do this?

balthazar · 2016-01-17T15:12:08Z

@oliviertassinari I got the Intl for the days abbreviations, but I dunno how to get the first day of the week without having to rely on the getDate method of the Date object, which doesn't seems to follow the current locale.

oliviertassinari · 2016-01-17T15:14:47Z

Sorry, this diff is unavailable.

😁

I got the Intl for the days abbreviations

Awesome!

but I dunno how to get the first day of the week without having to rely on the getDate method of the Date object, which doesn't seems to follow the current locale.

Yeah, I think that it's the most difficult point.

balthazar · 2016-01-17T15:18:38Z

Ah sorry, I thought rebasing would be better.

Any idea why the build cannot seems to find the Intl variable?

oliviertassinari · 2016-01-17T15:22:51Z

Any idea why the build cannot seems to find the Intl variable?

I can't see the line changes right now.

Regarding the latest point. Basically, we are looking for something like: http://momentjs.com/docs/#/get-set/iso-weekday/.

balthazar · 2016-01-17T15:25:16Z

Will try to fix the commit.

Hum yeah, I thought you didn't want to add an extra dependency like moment, but it would be pretty easy with it yes

oliviertassinari · 2016-01-17T15:30:31Z

I thought you didn't want to add an extra dependency like moment

I definitely want to avoid this. Here is their implementation https://github.com/moment/moment/blob/11b18a8c5e8a7970d38352f96f226c3351b89a43/src/lib/units/day-of-week.js#L154.

balthazar · 2016-01-17T15:58:17Z

Okay the commit is now fixed, I removed the merge so you can see the diff.

Also I looked a bit this method and actually don't know how we can use it to fit our needs (my mind is really bad with dates and timezone 😄), and do we really will have to re-implement each method we need to not include a library?

balthazar · 2016-01-18T10:34:40Z

@oliviertassinari I updated the changes to handle the build issue and use a fallback when Intl is not available. Would you consider merge this PR with the firstDayOfWeek parameter?

I'm thinking the changes with our own implementation of the first day detection would be out-of-range of this PR and might need a closer look to the possibilities

oliviertassinari · 2016-01-18T10:38:11Z

Would you consider merge this PR with the firstDayOfWeek parameter?

If we don't have better option, yes, and in this case, It would be great to update the fr example of the documentation with firstDayOfWeek=1.

oliviertassinari · 2016-01-18T10:40:13Z

src/utils/date-time.js

+    return new Date(now.setDate(now.getDate() - now.getDay()));
+  },
+
+  getWeekArray(d, firstDayOfWeek = 0) {


Can you set the default value in the getDefaultProps of the DateTimePicker component?
This will automatically be used by the documentation.

balthazar · 2016-01-18T11:23:20Z

@oliviertassinari I changed the things you pointed out ;)

balthazar · 2016-01-18T11:41:43Z

Wait I need to change something

balthazar · 2016-01-18T13:34:46Z

@oliviertassinari All good 👌 I didn't worked when you didn't provided a DateTimeFormat.

Sorry for all the pings 😕

oliviertassinari · 2016-01-18T16:54:10Z

src/date-picker/calendar.jsx

@@ -59,6 +60,7 @@ const Calendar = React.createClass({
      selectedDate: this.props.initialDate,
      transitionDirection: 'left',
      transitionEnter: true,
+      daysArray: [...Array(7)],


This will be allocated for each instance of the component. Why not simply move daysArray outside of the component?

Like a global variable? Where do you want to put it exactly?

Yes, you can do something like:

.... import ThemeManager from '../styles/theme-manager'; import DefaultRawTheme from '../styles/raw-themes/light-raw-theme'; const daysArray = [...Array(7)]; const Calendar = React.createClass({ ...

oliviertassinari · 2016-01-18T17:00:51Z

Could you add firstDayOfWeek={1} to the DatePickerExampleInternational example?
That we improve our current french example.

Regarding firstDayOfWeek:

I'm thinking the changes with our own implementation of the first day detection would be out-of-range of this PR and might need a closer look to the possibilities

I agree, this is a related issue: tc39/ecma402#6.

oliviertassinari · 2016-01-18T17:09:14Z

src/date-picker/date-picker.jsx

@@ -43,6 +43,11 @@ const DatePicker = React.createClass({
    disableYearSelection: React.PropTypes.bool,

    /**
+     * Used to change the first day of week.
+     */
+    firstDayOfWeek: React.PropTypes.number,


I think it would be good to add something link:
The first day of the week drastically varies between Saturday, Sunday, or Monday (or even Friday) between different locales.

balthazar · 2016-01-18T17:18:20Z

@oliviertassinari I changed the docs, comments and the daysArray variable.

Nice catch for ecma the issue, really interesting

oliviertassinari · 2016-01-18T17:22:07Z

@apercu Really nice PR 👍
@alitaheri @newoga Do you want to do a final review before merging?

mbrookes · 2016-01-18T17:23:21Z

Please could you note the use of the firstDayOfWeek property in docs/src/app/components/pages/components/DatePicker/Page.jsx in descriptions.localised?

alitaheri · 2016-01-18T18:57:36Z

src/date-picker/date-picker.jsx

+     * Used to change the first day of week. It drastically varies from
+     * Saturday to Monday (could even be Friday) between different locales.
+     */
+    firstDayOfWeek: React.PropTypes.number,


Might not be a bad idea to clarify what the default value, 0 means Sunday.

I haven't though about this. I agree 😁

Grrrr 😁 that's what I did in my first commit! 😄

Sorry 💫 !

Haha no problem I'm kidding

alitaheri · 2016-01-18T19:02:58Z

Great work @apercu, That one thing is my only concern. In my country the first day of week is Saturday. Although I know how america works. But I had to check the source to be sure. So a simple clarification can save the users a bit of time. Thanks for the awesome work 👍 👍

mbrookes · 2016-01-18T19:47:18Z

@alitaheri - that's an interesting point. IMHO without localisation applied, I believe Material-UI date handling should be "international" by default. There's an open issue to make the default date displayed follow ISO8601 (YYYY-MM-DD), as the US format is ambiguous everywhere, including in the US!

By the same token, I think the default the first day of the week should be also follow ISO8601. This happens to be Monday for better or worse, but at least it would be consistent with a standard, rather than following one country's cultural norm.

oliviertassinari · 2016-01-18T19:51:08Z

By the same token, I think the default the first day of the week should be also follow ISO8601. This happens to be Monday for better or worse, but at least it would be consistent with a standard

I agree, but we have to wait for the next breaking changes release.

alitaheri · 2016-01-18T20:38:01Z

@mbrookes I completely agree. Following the standard is the best possible way to tackle this. But as @oliviertassinari said, it will have to wait since it's a breaking change. so we need to find a way to ease into it or find a way to warn the users ahead of time if transition is not possible.

mbrookes · 2016-01-18T20:46:21Z

Yeah, don't want to stand in the way of this PR. Great work BTW @apercu!

newoga · 2016-01-18T20:52:57Z

@alitaheri @oliviertassinari Sorry, I've been absolutely slammed.If you both are satisfied, feel free to merge. Seems like there might be some topics to revisit but if the code quality is solid, I'm happy 😄

I'll review the changes either way tonight

Edit: Looked very quickly, this seems like it's pretty low risk. I'm good! Thanks @apercu!

balthazar · 2016-01-18T22:15:19Z

I updated the commit to match the review of @alitaheri.

For the point raised by @mbrookes I totally agree, but do you think we will remember to change this in the next breaking version?

[DatePicker] Add firstDayOfWeek and days abbreviations

oliviertassinari · 2016-01-18T22:40:15Z

@apercu Thanks 🎉 !

but do you think we will remember to change this in the next breaking version

I have added it here: #2109.

balthazar · 2016-01-18T22:43:58Z

@oliviertassinari @alitaheri @mbrookes @newoga Thanks for the positive feedbacks and your work! Happy to contribute 🎉

(Should be the last ping from me (on this PR!) 😄)

…d abbreviations When it's available

alitaheri · 2016-01-19T06:13:02Z

Thanks a lot, this was surely an important feature to have! 👍 🎉 🎉

oliviertassinari added the PR: Needs Review label Jan 12, 2016

oliviertassinari reviewed Jan 17, 2016
View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 17, 2016

balthazar force-pushed the master branch from 6bf9fb2 to c1d4422 Compare January 17, 2016 15:06

balthazar force-pushed the master branch from f15a6bf to a3fdc94 Compare January 17, 2016 15:50

balthazar force-pushed the master branch from a3fdc94 to 8271747 Compare January 18, 2016 10:27

oliviertassinari reviewed Jan 18, 2016
View reviewed changes

balthazar force-pushed the master branch from 8271747 to 9d6f5b8 Compare January 18, 2016 11:18

balthazar force-pushed the master branch from 9d6f5b8 to 84a027e Compare January 18, 2016 13:26

oliviertassinari reviewed Jan 18, 2016
View reviewed changes

balthazar force-pushed the master branch from 84a027e to f33a7da Compare January 18, 2016 17:16

oliviertassinari added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 18, 2016

balthazar force-pushed the master branch from f33a7da to e92c9fa Compare January 18, 2016 17:27

alitaheri reviewed Jan 18, 2016
View reviewed changes

balthazar force-pushed the master branch from e92c9fa to 2841775 Compare January 18, 2016 22:10

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Jan 18, 2016

oliviertassinari added a commit that referenced this pull request Jan 18, 2016

Merge pull request #2899 from Apercu/master

aa059c7

[DatePicker] Add firstDayOfWeek and days abbreviations

oliviertassinari merged commit aa059c7 into mui:master Jan 18, 2016

oliviertassinari mentioned this pull request Jan 18, 2016

v0.15.0 #2109

Closed

8 tasks

oliviertassinari mentioned this pull request Jan 18, 2016

Localized weekday titles in Date Picker + ability to specify weekday … #2282

Closed

feat(calendar): add ability to change firstDayOfWeek and use localize…

2841775

…d abbreviations When it's available

zannager added component: date picker This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module! labels Mar 15, 2023

feat(calendar): add firstDayOfWeek and days abbreviations #2899

feat(calendar): add firstDayOfWeek and days abbreviations #2899

Conversation

balthazar commented Jan 12, 2016

Choose a reason for hiding this comment

balthazar commented Jan 17, 2016

balthazar commented Jan 17, 2016

balthazar commented Jan 17, 2016

oliviertassinari commented Jan 17, 2016

balthazar commented Jan 17, 2016

oliviertassinari commented Jan 17, 2016

balthazar commented Jan 17, 2016

oliviertassinari commented Jan 17, 2016

balthazar commented Jan 17, 2016

balthazar commented Jan 18, 2016

oliviertassinari commented Jan 18, 2016

Choose a reason for hiding this comment

balthazar commented Jan 18, 2016

balthazar commented Jan 18, 2016

balthazar commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jan 18, 2016

Choose a reason for hiding this comment

balthazar commented Jan 18, 2016

oliviertassinari commented Jan 18, 2016

mbrookes commented Jan 18, 2016

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

alitaheri commented Jan 18, 2016

mbrookes commented Jan 18, 2016

oliviertassinari commented Jan 18, 2016

alitaheri commented Jan 18, 2016

mbrookes commented Jan 18, 2016

newoga commented Jan 18, 2016

balthazar commented Jan 18, 2016

oliviertassinari commented Jan 18, 2016

balthazar commented Jan 18, 2016

alitaheri commented Jan 19, 2016