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] Add short weekdays token #10988

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

alexfauquette
Copy link
Member

Fix master failing

@alexfauquette alexfauquette added the component: pickers This is the name of the generic UI component, not the React module! label Nov 10, 2023
@mui-bot
Copy link

mui-bot commented Nov 10, 2023

Deploy preview: https://deploy-preview-10988--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 3e2c5b9

@@ -30,7 +30,9 @@ export const testLocalization: DescribeGregorianAdapterTestSuite = ({ adapter })
const formatString = adapter.formats[formatKey];
const expandedFormat = cleanText(adapter.expandFormat(formatString));

if (expandedFormat === formatString) {
if (expandedFormat === formatString || formatString === 'ccccc') {
Copy link
Member

Choose a reason for hiding this comment

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

Hardcoding the value is not great but I don't have a good idea on how to improve it.
We can make a slightly more specific:

Suggested change
if (expandedFormat === formatString || formatString === 'ccccc') {
if (expandedFormat === formatString || (adapter.lib === 'luxon' && formatString === 'ccccc')) {
// Luxon format 'ccccc' is not supported by the field components since multiple day can have the same one-letter value (e.g: Tuesday and Thursday).
// It is used in the calendar header to display the day of the weeks.

A cleaner approach might be to add entries to the formatTokenMap saying "notSupported` to have a clean error instead of escaping it in Luxon.
But it will improve the bundle size a bit for something not common.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but in that case we should also think about this behavior for other adapters

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about other adapters that will have the same issue of a token used into the adapter format that's not into the formatTokenMap

The only other example I found is for date-fns: weekdayShort: 'EEEEEE'

For consistency if in luxon we warn that ccccc is not supported, we should do the same with EEEEE for date fns

Copy link
Member

Choose a reason for hiding this comment

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

But for the other adapters, since we don't escape the token in the adapter (like on Luxon), shouldn't it work already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I miss understood your proposal.

For me, you were proposing to add in the formatTokenMap something like ccccc: { sectionType: 'weekDay', contentType: "unsupported token" }

Copy link
Member

Choose a reason for hiding this comment

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

Something like that yes (not sure about the exact DX).

What I was saying is that for all the adapters except Luxon, the unsupported tokens should have a clean warning thrown by getDateSectionConfigFromFormatToken.
From what I understand, we now escape the non-token words in AdapterLuxon based on the token list we know. So for Luxon, we risk to escape words that are actually token we don't know. But this is not something that can occur on the other adapters.

So maybe just having a string array in AdapterLuxon with the non-supported token would be enough, so that we can avoid to escape them as well, and have getDateSectionConfigFromFormatToken throw the error properly.

Or maybe I'm missing something

Co-authored-by: Flavien DELANGLE <flaviendelangle@gmail.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
@alexfauquette alexfauquette enabled auto-merge (squash) November 10, 2023 10:18
@alexfauquette alexfauquette merged commit 8364e2a into mui:next Nov 10, 2023
16 checks passed
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants