Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1179 - Datepicker: Ability to set first day of week #1326

Merged
merged 6 commits into from
Dec 10, 2018

Conversation

ericangeles
Copy link
Contributor

Explain the details for making this change. What existing problem does the pull request solve?

Adding a test template that will control the first day on the datepicker.
Adding a firstDayofWeek option to datepicker that overrides whats used in locale.

Related github/jira issue (required):

Closes #1179

Steps necessary to review your pull request (required):

Copy link
Contributor

@EdwardCoyle EdwardCoyle left a comment

Choose a reason for hiding this comment

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

@ericangeles see below


<script>
$('body').on('initialized', function () {
Locale.currentLocale.data.calendars[0].firstDayofWeek = 1; // starts on Monday
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this type of fix is in "workaround" territory, only because it could potentially be unstable to the Locale system if it's using this property internally for other calendar functions. #1179 actually lists out that a Datepicker API setting for changing the first day of the week would be desirable. I'm thinking that would be a better fix.

Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Sorry @ericangeles should have made this one more clear. To add some direction:

  1. add a setting firstDayOfWeek on the monthview component and datepicker component (datepicker one just sends it to month view)
  2. its initially null, if not set use Locale firstDayOfWeek but if it is set use the setting value instead of whats set in the locale (dont change the locale)
  3. add api tests ext.
  4. update the example to show the setting not the workaround

<script>
$('body').on('initialized', function () {
Locale.currentLocale.data.calendars[0].firstDayofWeek = 1; // starts on Monday
$('my-datepicker').datepicker();
Copy link
Member

Choose a reason for hiding this comment

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

Should prob be #date-field-normal

Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

  1. Change as noted to add docs
  2. Change as noted to not change the local and instead override in month view
  3. Add a functional test
  4. Merge master on to fix build problems.

src/components/datepicker/datepicker.js Outdated Show resolved Hide resolved
src/components/monthview/monthview.js Show resolved Hide resolved
src/components/datepicker/datepicker.js Show resolved Hide resolved
@tmcconechy
Copy link
Member

In addition to our comments will want to merge master to fix the tests

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

Successfully merging this pull request may close these issues.

None yet

3 participants