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.

Sign up for GitHub

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

Jump to bottom

feat: let users configure their first day of week #46592

Merged
merged 1 commit into from
Jul 23, 2024
Merged

feat: let users configure their first day of week #46592

merged 1 commit into from
Jul 23, 2024

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Jul 17, 2024 •
edited
Loading

Summary

The property will be propagated to the whole ecosystem via window.firstDay and our l10n library respectively.

Automatic

Bildschirmfoto_20240718_082159

Overwritten

Bildschirmfoto_20240718_082121

Popover open

Bildschirmfoto_20240718_081938

TODO

  • Add screenshots
  • Fix tests etc.

Checklist

@st3iny st3iny added this to the Nextcloud 30 milestone Jul 17, 2024
@st3iny st3iny self-assigned this Jul 17, 2024
@st3iny st3iny added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 18, 2024
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 1cabe94 to 7d4d200 Compare July 18, 2024 06:25
@st3iny st3iny marked this pull request as ready for review July 18, 2024 06:25
@st3iny st3iny requested a review from Pytal July 18, 2024 06:26
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 7d4d200 to 4674699 Compare July 18, 2024 06:35
ChristophWurst
ChristophWurst reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Smooth 😎

I think we only need Sunday+Monday as options. Are the other days used in any culture?

apps/provisioning_api/lib/Controller/UsersController.php
case self::USER_FIELD_FIRST_DAY_OF_WEEK:
$intValue = (int)$value;
if ($intValue < -1 || $intValue > 6) {
throw new OCSException($this->l10n->t('Invalid first day of week'), 102);
Copy link
Member

Choose a reason for hiding this comment

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

magic number 102?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just like the other properties. For some reason they all return 102 in case of invalid values 🤷‍♂️

artonge
artonge reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Should we keep the "Week starts on ..." below the "Locale" setting?

apps/settings/lib/Settings/Personal/PersonalInfo.php Outdated Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/FirstDayOfWeekSection.vue Show resolved Hide resolved
@artonge
Copy link
Contributor

artonge commented Jul 18, 2024

I think we only need Sunday+Monday as options. Are the other days used in any culture?

I imagine this is also useful to people that do not have usual weekend days, so I would keep the full list of days.

@ChristophWurst
Copy link
Member

ChristophWurst commented Jul 18, 2024 •
edited
Loading

The start day is also used in the date picker. I'm not sure it can even cope with Wednesday as start day. Something to test?

https://github.com/nextcloud-libraries/nextcloud-vue/blob/30506f203f5cbb505e6b58ecde98456ecb027881/src/components/NcDateTimePicker/NcDateTimePicker.vue#L311-L312

@st3iny
Copy link
Member Author

st3iny commented Jul 18, 2024

The start day is also used in the date picker. I'm not sure it can even cope with Wednesday as start day. Something to test?

I tested it in Calendar and it copes fine.

grafik

ChristophWurst
ChristophWurst approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good

@st3iny st3iny requested a review from artonge July 22, 2024 13:43
@st3iny st3iny mentioned this pull request Jul 22, 2024
Open
@st3iny
feat: let users configure their first day of week
b37fb43 
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 76ff158 to b37fb43 Compare July 22, 2024 20:37
@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 22, 2024
@st3iny st3iny enabled auto-merge July 22, 2024 20:37
@st3iny st3iny merged commit ee6f857 into master Jul 23, 2024
167 checks passed
@st3iny st3iny deleted the feat/first-day-of-week-user-setting branch July 23, 2024 07:03
@tcitworld
Copy link
Member

I think we only need Sunday+Monday as options. Are the other days used in any culture?

There's also Friday and Saturday : https://en.wikipedia.org/wiki/Week

@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@st3iny st3iny mentioned this pull request Sep 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@come-nc come-nc Awaiting requested review from come-nc

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@st3iny st3iny

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: language l10n and translations feature: settings
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

Starting day of the week (Sunday vs Monday)
4 participants
@st3iny @artonge @ChristophWurst @tcitworld