Feature: add firstDayOfWeek prop #371

daltones · 2017-03-13T20:58:25Z

This PR tries to provide a solution to fix #363.

We urgently needed this feature in our app. Right now a forked react-dates with this implementation is running in production, all good. We use just <DayPicker /> components in two different pages.

~~For now it's missing passing along prop firstDayOfWeek from DateRangePicker and SingleDatePicker and write some new tests for this functionality.~~

The big change here is the [almost] rewrite of getCalendarMonthWeeks. The logic now is that every operation involving weekdays needs the firstDayOfWeek parameter (0-Sun...6-Sat locale independent). And then firstDayOfWeek prop from components gets the default value according to Moment's global locale (so we're not breaking anything).

I'd be happy if I receive feedback of this implementation. I'll update the PR very soon with the rest.

daltones · 2017-03-13T21:09:52Z

Oh, I forgot to mention. This implementation also fixes a bug in an edge case I found:

// We have Moment's global locale starting the week on Sundays
moment.locale('some-sunday-based-locale');

// For some reason in initialVisibleMonth we return a moment object set
// with another locale that starts the week on Mondays
<DayPicker initialVisibleMonth={() => moment().locale('some-monday-based-locale')} />

In react-dates v8.1.1 we'll have a calendar that shows the weekdays header starting on Monday, but the actual days arranged as starting on Sunday.

coveralls · 2017-03-13T21:20:25Z

Coverage decreased (-0.2%) to 81.981% when pulling 092e01c on daltones:feat/first-weekday into abd0b1b on airbnb:master.

ljharb

A few comments on the implementation.

Looks good overall tho, but it requires test coverage on all affected files.

ljharb · 2017-03-14T00:41:28Z

src/components/CalendarMonth.jsx

@@ -44,6 +45,7 @@ const defaultProps = {
  onDayMouseEnter() {},
  onDayMouseLeave() {},
  renderDay: null,
+  firstDayOfWeek: moment.localeData().firstDayOfWeek(),


this will freeze the default at parse time (and the locale could be changed at any time) - i think this default needs to be calculated in the render path, and have it default to -1 or null here.

Yeah, sure. I think null is better.

ljharb · 2017-03-14T00:41:41Z

src/components/CalendarMonthGrid.jsx

@@ -52,6 +53,7 @@ const defaultProps = {
  onMonthTransitionEnd() {},
  renderDay: null,
  transformValue: 'none',
+  firstDayOfWeek: moment.localeData().firstDayOfWeek(),


ljharb · 2017-03-14T00:43:03Z

src/components/DayPicker.jsx

@@ -65,6 +66,7 @@ const defaultProps = {
  onOutsideClick() {},
  hidden: false,
  initialVisibleMonth: () => moment(),
+  firstDayOfWeek: moment.localeData().firstDayOfWeek(),


ljharb · 2017-03-14T00:44:17Z

src/utils/getCalendarMonthWeeks.js

+export default function getCalendarMonthWeeks(
+  month,
+  enableOutsideDays,
+  firstDayOfWeek = moment.localeData().firstDayOfWeek(),


can something validate that this value is an integer 0-6, and throw a TypeError if not?

I thought about that, but I didn't see any other example of this kind of validation. Even here we call month.clone() without knowing if month is in fact a moment object. Moreover, like all these parameters of utils functions, values came from component props that already pass by some propTypes validation in dev.

The propType should ensure that. I think that in general the utils functions should not assume safe inputs.

coveralls · 2017-03-23T01:53:05Z

Coverage increased (+0.1%) to 87.436% when pulling b62a3aa on daltones:feat/first-weekday into b90ee78 on airbnb:master.

ljharb · 2017-03-23T05:42:10Z

@daltones would you please rebase this branch to ensure there's no merge commits in it?

coveralls · 2017-03-24T03:27:30Z

Coverage increased (+0.1%) to 87.436% when pulling 4b38335 on daltones:feat/first-weekday into b90ee78 on airbnb:master.

daltones · 2017-03-24T03:29:00Z

@ljharb done!

coveralls · 2017-03-24T03:31:34Z

Coverage increased (+0.1%) to 87.436% when pulling 4b38335 on daltones:feat/first-weekday into b90ee78 on airbnb:master.

ljharb · 2017-03-24T03:43:20Z

src/components/CalendarMonth.jsx

+      weeks: getCalendarMonthWeeks(
+        props.month,
+        props.enableOutsideDays,
+        props.firstDayOfWeek === null ? moment.localeData().firstDayOfWeek() : props.firstDayOfWeek,


This should probably use == null to avoid distinguishing from undefined

Okay, I can do this. But you're saying just because code style? If firstDayOfWeek has a defaultValue set to null, it would never be undefined, right?

I do think you're right; that a value of undefined would trigger the default, and a value of null would not, so === null would work fine - but in general for style, yes. Not a huge deal, ofc.

ljharb · 2017-03-24T03:43:29Z

src/components/CalendarMonth.jsx

+        weeks: getCalendarMonthWeeks(
+          month,
+          enableOutsideDays,
+          firstDayOfWeek === null ? moment.localeData().firstDayOfWeek() : firstDayOfWeek,


ljharb · 2017-03-24T03:43:44Z

src/components/DayPicker.jsx

@@ -350,11 +352,16 @@ export default class DayPicker extends React.Component {

    const style = this.isHorizontal() ? horizontalStyle : {};

+    let { firstDayOfWeek } = this.props;
+    if (firstDayOfWeek === null) {


ljharb · 2017-03-24T03:44:48Z

src/components/DayPickerRangeController.jsx

@@ -52,6 +52,7 @@ const propTypes = forbidExtraProps({
  onOutsideClick: PropTypes.func,
  renderDay: PropTypes.func,
  renderCalendarInfo: PropTypes.func,
+  firstDayOfWeek: PropTypes.oneOf([0, 1, 2, 3, 4, 5, 6]),


I think this propType should be shared and exported up from the components that need it - ie, this component shouldn't du-locate the definition, it should just import the propType from DayPicker and delegate the authority to it.

Same applies to any component where the prop is merely proxied down.

Effectively I'd prefer the oneOf to only appear in one place.

ljharb · 2017-03-24T03:46:39Z

src/utils/getCalendarMonthWeeks.js

-  const currentDay = firstOfMonth.clone();
-  let currentWeek = [];
-  const weeksInMonth = [];
+const WEEKDAYS = [0, 1, 2, 3, 4, 5, 6];


Could this array be shared, and used inside the oneOf?

coveralls · 2017-03-24T04:29:48Z

Coverage increased (+0.2%) to 87.563% when pulling c2c333f on daltones:feat/first-weekday into b90ee78 on airbnb:master.

@majapw

code LGTM. deferring to @majapw

CarlRosell · 2017-06-08T08:55:19Z

What's the current status of this PR?
I'm glad to see someone else also having this issue. I've made the same changes for other "date-picker"-libraries as this is a major blocker for the company where I work. 👍

majapw · 2017-06-09T04:54:25Z

Hi @CarlRosell, I can take a look at this tomorrow/early next week.

daspete · 2017-07-06T15:16:31Z

Hi there, is this feature in the todo pipeline? It would be great to see this one working, because our company need to set the first day to monday.... or is there another way to set this?

majapw

Seems legit to me!

majapw · 2017-07-14T22:05:27Z

@ljharb since I updated this PR with my own changes/rebase/etc. can you please sanity-check it for me before I merge it in?

coveralls · 2017-07-14T22:11:01Z

Coverage increased (+0.04%) to 86.243% when pulling dfbf84e on daltones:feat/first-weekday into 1031985 on airbnb:master.

coveralls · 2017-07-14T22:16:33Z

Coverage increased (+0.04%) to 86.243% when pulling 071d78b on daltones:feat/first-weekday into 1031985 on airbnb:master.

ljharb

LGTM pending the one test change

ljharb · 2017-07-14T22:37:09Z

test/utils/getCalendarMonthWeeks_spec.js

+    });
+
+    describe('locale with Monday as first day of week', () => {
+      before(() => {


we need to use beforeEach here; before is beforeAll, and we don't want that.

ljharb · 2017-07-14T22:37:09Z

test/utils/getCalendarMonthWeeks_spec.js

+    const january2017End = january2017Start.clone().endOf('month'); // Tuesday
+
+    describe('locale with Sunday as first day of week', () => {
+      before(() => {


we need to use beforeEach here; before is beforeAll, and we don't want that.

coveralls · 2017-07-14T22:52:14Z

Coverage increased (+0.04%) to 86.243% when pulling 55c7d37 on daltones:feat/first-weekday into 19a10f7 on airbnb:master.

coveralls · 2017-07-14T22:54:01Z

Coverage increased (+0.04%) to 86.243% when pulling 55c7d37 on daltones:feat/first-weekday into 19a10f7 on airbnb:master.

majapw · 2017-07-14T22:55:17Z

@ljharb addressed your comments!

ljharb

K last thing, I swear :-)

ljharb · 2017-07-15T00:36:48Z

src/shapes/DayOfWeekShape.js

@@ -0,0 +1,5 @@
+import { PropTypes } from 'react';


This needs to use the prop-types package

…stDayOfWeek prop working with the SDP again

majapw · 2017-07-15T02:31:04Z

Naw this is good! You're catching all the things I've missed. :) I addressed the prop types issue @ljharb, can you take another look?

coveralls · 2017-07-15T02:40:56Z

Coverage increased (+0.04%) to 86.243% when pulling 139e274 on daltones:feat/first-weekday into 19a10f7 on airbnb:master.

CarlRosell · 2017-07-15T17:26:42Z

🎉

…weekday generated from commit f149c68

cesalberca · 2017-07-19T13:56:05Z

Nice! Just in time for a component I'm doing in spanish ^^ Any ideas when this patch will be released?

majapw · 2017-07-24T16:50:27Z

Should be live in v12.3.0! :)

closes #128 Required an upgrade of react-dates, to include react-dates/react-dates#371

ljharb requested changes Mar 14, 2017

View reviewed changes

ljharb added the semver-minor: new stuff Any feature or API addition. label Mar 14, 2017

daltones force-pushed the feat/first-weekday branch 2 times, most recently from d570d5b to 4b38335 Compare March 24, 2017 03:15

ljharb previously requested changes Mar 24, 2017

View reviewed changes

ljharb requested a review from majapw March 24, 2017 05:26

majapw force-pushed the feat/first-weekday branch 2 times, most recently from dfbf84e to 071d78b Compare July 14, 2017 22:04

majapw previously approved these changes Jul 14, 2017

View reviewed changes

ljharb requested changes Jul 14, 2017

View reviewed changes

majapw dismissed their stale review via a870669 July 14, 2017 22:41

majapw force-pushed the feat/first-weekday branch from 071d78b to a870669 Compare July 14, 2017 22:41

daltones added 2 commits July 14, 2017 15:43

Feature: prop firstDayOfWeek

b65541c

Expose DayOfWeekShape propType

7475cd2

majapw force-pushed the feat/first-weekday branch from a870669 to 55c7d37 Compare July 14, 2017 22:43

ljharb requested changes Jul 15, 2017

View reviewed changes

src/shapes/DayOfWeekShape.js Outdated

@@ -0,0 +1,5 @@

import { PropTypes } from 'react';

Copy link

Member

ljharb Jul 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use the prop-types package

Fix conflicts with master, add examples and documentation and get fir…

139e274

…stDayOfWeek prop working with the SDP again

majapw force-pushed the feat/first-weekday branch from 55c7d37 to 139e274 Compare July 15, 2017 02:30

ljharb approved these changes Jul 15, 2017

View reviewed changes

majapw merged commit f149c68 into react-dates:master Jul 15, 2017

cchanningallen pushed a commit to cchanningallen/react-dates that referenced this pull request Jul 17, 2017

publish: Merge pull request react-dates#371 from daltones/feat/first-…

cf03299

…weekday generated from commit f149c68

drewda added a commit to opentraffic/analyst-ui that referenced this pull request Sep 28, 2017

Adjust calendar UI so week starts on Monday

2e9da31

closes #128 Required an upgrade of react-dates, to include react-dates/react-dates#371

drewda mentioned this pull request Sep 28, 2017

Adjust calendar UI so week starts on Monday opentraffic/analyst-ui#130

Merged

daltones mentioned this pull request Sep 21, 2023

Feature: prop firstDayOfTheWeek #363

Closed

Feature: add firstDayOfWeek prop #371

Feature: add firstDayOfWeek prop #371

Conversation

daltones commented Mar 13, 2017 • edited by ljharb Loading

daltones commented Mar 13, 2017 • edited Loading

coveralls commented Mar 13, 2017

ljharb left a 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

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

coveralls commented Mar 23, 2017

ljharb commented Mar 23, 2017

coveralls commented Mar 24, 2017

daltones commented Mar 24, 2017

coveralls commented Mar 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 24, 2017

CarlRosell commented Jun 8, 2017

majapw commented Jun 9, 2017

daspete commented Jul 6, 2017

majapw left a comment

Choose a reason for hiding this comment

majapw commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 14, 2017

coveralls commented Jul 14, 2017

majapw commented Jul 14, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Jul 15, 2017

coveralls commented Jul 15, 2017

CarlRosell commented Jul 15, 2017

cesalberca commented Jul 19, 2017

majapw commented Jul 24, 2017

daltones commented Mar 13, 2017 •

edited by ljharb

Loading

daltones commented Mar 13, 2017 •

edited

Loading