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] Adds RTL verification for the time pickers default views #13447

Merged

Conversation

arthurbalduini
Copy link
Member

Closes #11525

@arthurbalduini arthurbalduini added component: pickers This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Jun 11, 2024
@mui-bot
Copy link

mui-bot commented Jun 11, 2024

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

Generated by 🚫 dangerJS against f65ac35

@arthurbalduini arthurbalduini force-pushed the pickers/views-default-value-on-rtl branch from 7128d07 to cc12d23 Compare June 12, 2024 07:11
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.

It should also apply to the plain MultiSectionDigitalClock.
Screenshot 2024-06-12 at 10 35 02

packages/x-date-pickers/src/TimePicker/shared.tsx Outdated Show resolved Hide resolved
@LukasTy
Copy link
Member

LukasTy commented Jun 12, 2024

I feel like it should be doable within MultiSectionDigitalClock using style adjustments.
Have you considered a flex order?

@flaviendelangle flaviendelangle changed the title [timepickers][fields] Adds RTL verification for field default views [pickers] Adds RTL verification for the time pickers default views Jun 17, 2024
@arthurbalduini arthurbalduini force-pushed the pickers/views-default-value-on-rtl branch 2 times, most recently from 8b27471 to 9b529fb Compare June 17, 2024 11:41
@LukasTy LukasTy added the component: TimePicker The React component. label Jun 17, 2024
}

const viewsWithoutMeridiem = inViews.filter((view) => view !== 'meridiem');
return viewsWithoutMeridiem.toReversed().concat('meridiem');
Copy link
Member

Choose a reason for hiding this comment

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

We are playing with fire here regarding views. 🙈
Making such structural views adjustments is tricky and this is the place where such change creates the most problems. 🙈
The tabulation/views process does not work as expected in RTL mode.
Given a demo with default settings, the minutes view is skipped—selecting hours jumps the focus to meridiem and after selecting it the Picker closes.

Have you tried adjusting only the views intended for rendering?

@Eliav2
Copy link

Eliav2 commented Jun 17, 2024

Dude I'm not sure what you changed there but it looks overkill
Want me to make a PR ?

@LukasTy
Copy link
Member

LukasTy commented Jun 17, 2024

Dude I'm not sure what you changed there but it looks overkill
Want me to make a PR ?

If you have ideas on how to do it—the suggestion are welcome.
I imagine, that given your suggestion, all the time sections, except for meridiem should be wrapped in a single element and then your suggestion should apply.
Screenshot 2024-06-17 at 20 21 01

Or do you @Eliav2 have a suggestion on how to achieve it without a breaking DOM change? 🤔

@arthurbalduini arthurbalduini force-pushed the pickers/views-default-value-on-rtl branch from 9b529fb to ef7c577 Compare June 18, 2024 09:51
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.

Great work! 👏

We are snapshot testing docs only in ltr mode, so, any regressions on this regard might go unnoticed.
WDYT about adding an e2e regression test for rtl behavior? 🤔

Comment on lines 408 to 409
(res, currentView) => {
return { ...res, [currentView]: buildViewProps(currentView) };
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: just to avoid unnecessary diff. 🙈

Suggested change
(res, currentView) => {
return { ...res, [currentView]: buildViewProps(currentView) };
(result, currentView) => {
return { ...result, [currentView]: buildViewProps(currentView) };

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! component: TimePicker The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] MultiSectionDigitalClock incorrect RTL behavior
4 participants