Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Fix firstDayOfWeek bug #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

g1mn
Copy link

@g1mn g1mn commented Mar 28, 2017

Fix firstDayOfWeek bug #21

@dreamalligator
Copy link
Contributor

confirmed this fixes!

@dreamalligator dreamalligator mentioned this pull request Apr 13, 2017
@nmocruz
Copy link

nmocruz commented Jul 4, 2017

Not sure if this line is causing the problem to

https://github.com/sensortower/daterangepicker/blob/master/src/scripts/daterangepicker/config.coffee#L37

if val is null, it set 0 and fire the update, that can be a different value and cause and update on the moment.localeData

myabe we need to used ko.observable(val ? val : moment.localeData().firstDayOfWeek())

@mbrodala
Copy link

mbrodala commented Feb 1, 2018

Seems like something deeper is broken here: this fix works for Day but doesn't affect Week at all. 🤔

@mbrodala
Copy link

mbrodala commented Feb 2, 2018

After some deeper investigation we found out that the behavior of the daterangepicker is far from optimal here. It always overwrites the firstDayOfWeek in the moment.js locale data with its own setting, ignoring the default provided by the selected locale.

Thus the proper fix for now is this:

moment.locale('de'); // Example: German
$element.daterangepicker({
  // ...
  firstDayOfWeek: moment.localeData().firstDayOfWeek(),
  // ...
});

This ensures that

  1. The table of days in the Day and Week view is correct and
  2. The calendar column headers (weekdays) match their days in the view.

This pull request is not a solution ATM.

@joehannouch
Copy link

Any updates on this matter?
Applied the change and indeed it only affects the Days, not working with Weeks view.

firstDayOfWeek : 1 simply shows a wrong calendar (although clicking on them shows a correct date).

@duyanhth
Copy link

I have this issue and can workaround (actually it can be a fix) by manual update moment plugin setting.

image

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

Successfully merging this pull request may close these issues.

None yet

6 participants