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

[pickers] Support changing start of week day on AdapterLuxon #10805

Closed
LukasTy opened this issue Oct 26, 2023 · 6 comments · Fixed by #10964
Closed

[pickers] Support changing start of week day on AdapterLuxon #10805

LukasTy opened this issue Oct 26, 2023 · 6 comments · Fixed by #10964
Labels
component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature l10n localization

Comments

@LukasTy
Copy link
Member

LukasTy commented Oct 26, 2023

Created from: #10790 (comment)

Do I understand it correctly that after the recent change, we no longer support changing the start of the weekday on Luxon adapter by overriding the getWeekdays method?

Yes, it's an unexpected regression that we must adress somehow I guess. I would like us to discuss if we have a better solution than this override. Two APIs that I can think about:

  • a prop on LocalizationProvider to set the start of the date that would only be used by Luxon, just like moment uses the dateLibInstance prop
  • a new static method on AdapterLuxon (AdapterLuxon.setCustomWeekStart) that returns a Luxon adapter with the correct start of week

There is also the concern of whether we are willing to risk breaking something we don't know.
Luxon currently doesn't support such a feature natively, because the Intl API doesn't have the full support of getWeekInfo: moment/luxon#954

Considering our position as "consumers" of the given library, we could simply refrain from coming up with custom implementations until a native one is provided by luxon. 🤔

Relevant issue and a proposed custom adapter: #7670 (comment) that we basically broke with: #10345.

Search keywords:

@LukasTy LukasTy added l10n localization component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels Oct 26, 2023
@flaviendelangle
Copy link
Member

There is also the concern of whether we are willing to risk breaking something we don't know.

We could provide the method as unstable AdapterLuxon.unstable_setCustomWeekStart.
It would still be more reliable than an adapter method override, but at the same time we could clearly state that we are in an unknown territory.

@carvalheiro
Copy link

carvalheiro commented Nov 8, 2023

Hey guys! Do we have any sense of when would this be fixed? I'd be open to work on the change if necessary too 🚀

@flaviendelangle
Copy link
Member

Hi,

Do you need any of the features released since this problem has been introduced ?
We started the alpha for the next major and this new method would probably go into it 👍

@carvalheiro
Copy link

No, right now we've been using a version that allows the workaround, so we can wait a bit for this implementation. But we will be upgrading it as soon as it released 🚀

@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 16, 2023

The clean fix will be merged in the v7 alpha branch.
I don't know yet if we will cherry-pick it on v6.

Bu you can apply the exact same behavior [like on this codesandbox](https://codesandbox.io/s/date-
If you encounter any issue, please let me know so that I can fix our implementation.and-time-pickers-forked-ffkc87?file=/src/App.tsx).
You just need Luxon 3.4.4 or above 👍

If you encounter any issue, please let me know so that I can fix our implementation.

@hokwanhung
Copy link

In case anyone needs the above URL provided by flaviendelangle (broken via GitHub Issues), below is the target sandbox:

Edit Date and time pickers (forked)

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! enhancement This is not a bug, nor a new feature l10n localization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants