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

Add first weekday option in profile #13991

Merged
merged 15 commits into from Oct 10, 2022
Merged

Add first weekday option in profile #13991

merged 15 commits into from Oct 10, 2022

Conversation

piitaya
Copy link
Member

@piitaya piitaya commented Oct 4, 2022

Proposed change

This PR allow user to select the first day of the week manually or use language option.

The language option uses Int.Locale.weekInfo but it's not always possible to determine the first day of week because it's not supported in all browsers and no polyfills exist. https://caniuse.com/mdn-javascript_builtins_intl_locale_weekinfo
So, a fallback has been added to https://github.com/gamtiq/weekstart for browsers that don't support Int.Locale.weekInfo.

Capture d’écran 2022-10-05 à 11 34 06

Fixed time components:

  • date input
  • date range picker
  • schedule
  • calendar
  • automation time condition (re-order days)

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@piitaya piitaya changed the title First weekday profile Add first weekday option in profile Oct 4, 2022
@@ -224,6 +224,7 @@ export const provideHass = (
language: localLanguage,
number_format: NumberFormat.language,
time_format: TimeFormat.language,
first_weekday: Weekday.monday,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
first_weekday: Weekday.monday,
first_weekday: Weekday.language,

if default, use:
"weekInfo" in Intl.Locale.prototype ? getLocaleFirstWeekDay(language) : Weekday.monday

Copy link
Member Author

@piitaya piitaya Oct 5, 2022

Choose a reason for hiding this comment

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

I don't think we can do this. If user select "language", it will have the right day on compatible browsers and Monday on others (firefox).

Copy link
Member Author

@piitaya piitaya Oct 5, 2022

Choose a reason for hiding this comment

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

We can fallback to this package if needed https://github.com/gamtiq/weekstart (or defined our own mapping locale => first day). But I think the best solution is to define Monday for everyone and let the user choose in profile.

Copy link
Member

Choose a reason for hiding this comment

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

weekstart is pretty small, I think we can use that as fallback?

Copy link
Member Author

@piitaya piitaya Oct 5, 2022

Choose a reason for hiding this comment

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

Yep, I just tested it and it don't impact the app size.
Most country/country use Monday as country, it just need to add the custom cases.

@piitaya piitaya marked this pull request as ready for review October 5, 2022 08:37
Comment on lines +16 to +19
// @ts-ignore
if ("weekInfo" in Intl.Locale.prototype) {
// @ts-ignore
return new Intl.Locale(locale.language).weekInfo.firstDay % 7;
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 added ts-ignore until typescript version is bump de 4.5+

Comment on lines +22 to +28
monday = "monday",
tuesday = "tuesday",
wednesday = "wednesday",
thursday = "thursday",
friday = "friday",
saturday = "saturday",
sunday = "sunday",
Copy link
Member

Choose a reason for hiding this comment

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

Should we make these numeric values, so we can use them as index directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is language option. So I preferred to keep all in string for consistency.

@bramkragten bramkragten merged commit 907466d into dev Oct 10, 2022
@bramkragten bramkragten deleted the first_weekday_profile branch October 10, 2022 14:58
@frenck frenck added the WTH Issues & PRs generated from the "Month of What the Heck?" label Oct 26, 2022
@poudenes
Copy link

Is this still in scope? Would be great to have Monday a as first dat of week. Most of the counties have Monday is start of the week. The United States, Canada, most of South America, China, Japan and the Philippines officially consider Sunday to start the week ahead.

@piitaya
Copy link
Member Author

piitaya commented Jul 18, 2023

This feature was added in 2022.11 release.

@poudenes
Copy link

Didn't catch that one. Maybe I saw it but didn't linked to it as issue... thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hacktoberfest WTH Issues & PRs generated from the "Month of What the Heck?"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date range picker not respecting locale DateTime picker first day of week should follow locale
5 participants