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

[docs] Add a doc section on how to override the start of the week with each adapter #11223

Merged
merged 9 commits into from
Dec 6, 2023

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 28, 2023

Closes #9785

I initially added demos but since the locale configuration is global, it impacts all the other demos 😬

@flaviendelangle flaviendelangle added docs Improvements or additions to the documentation component: pickers This is the name of the generic UI component, not the React module! labels Nov 28, 2023
@flaviendelangle flaviendelangle self-assigned this Nov 28, 2023
@@ -80,31 +80,6 @@ import 'moment/locale/de';

{{"demo": "LocalizationMoment.js"}}

:::warning
Copy link
Member Author

Choose a reason for hiding this comment

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

We are no longer using any moment method that are not localized.

@mui-bot
Copy link

mui-bot commented Nov 28, 2023

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice addition to the doc. 👍

I've just now checked that we might want to reconsider the release of this Luxon localized feature, because it's not going to work on Firefox. 🙈 🤯
https://moment.github.io/luxon/#/intl?id=locale-based-weeks
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/getWeekInfo#browser_compatibility
WDYT? On one hand, it's still an improvement for most, but on the other hand, it could be an annoying issue for others. 🤔
Maybe we should at least add a callout informing that currently Firefox does not support the localized Luxon behavior? 🤔
P.S. I was unable to locate an active firefox issue tracking the progress of this feature delivery. 🤷


## Custom start of week

The Date and Time Pickers are using the week settings provided by your date libraries.
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
The Date and Time Pickers are using the week settings provided by your date libraries.
The Date and Time Pickers are using the week settings provided by the date library you are using.

The Date and Time Pickers are using the week settings provided by your date libraries.
Each adapter uses its locale to define the start of the week.

If the default start of the week defined in your adapter's locale is not the one you want, you can override it as shown in the following examples.
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
If the default start of the week defined in your adapter's locale is not the one you want, you can override it as shown in the following examples.
If the default start of the week provided by your adapter's locale is not the one you want, you can override it as shown in the following examples.

@flaviendelangle
Copy link
Member Author

I've just now checked that we might want to reconsider the release of this Luxon localized feature, because it's not going to work on Firefox. 🙈 🤯

😭
I can look how it behaves on Firefox, but I guess we can make it opt-in and wait for Firefox to support it

@LukasTy
Copy link
Member

LukasTy commented Nov 29, 2023

I can look how it behaves on Firefox

Well, it simply doesn't react to the language/locale. Having English(en-us) set still renders the calendar the ISO way (starts on Monday).

@flaviendelangle
Copy link
Member Author

How do you think we should handle this feature then?

  • Keep if opt-out
  • Make it opt-in
  • Totally revert the changes

@LukasTy
Copy link
Member

LukasTy commented Nov 29, 2023

  • Make it opt-in

This option makes the most sense to me.
WDYT? 🤔

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Nov 29, 2023

I'm fine with this approach.

Would you do something like we proposed originally in this API ?

<LocalizationProvider adapter={AdapterLuxon.withLocaleWeeks()}>
</LocalizationProvider>

Or do you have another API in mind?

@LukasTy
Copy link
Member

LukasTy commented Nov 29, 2023

Would you do something like we proposed originally in this API ?

The proposed approach does seem the least intrusive, I like it. 👌

@flaviendelangle
Copy link
Member Author

We decided to keep the Luxon behavior opt-out for 4 reasons;

  1. The market share of Firefox is low
  2. Firefox will very likely support it soon
  3. If the make it opti-in, we will have to do a breaking change to everybody to make it opt-out in the future
  4. The component is usable in Firefox, the start of the week is just not the same

We are adding a warning on the doc to show people how to manually set the start of the week.

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM! 💯 🚀

@flaviendelangle flaviendelangle merged commit 8dff180 into mui:next Dec 6, 2023
@flaviendelangle flaviendelangle deleted the start-of-week-docs branch December 6, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Add documentation detailing the override of start of the week with different adapters
3 participants