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] Fix keyboard navigation with yearsOrder="desc" and direction="rtl" #14682

Merged

Conversation

thomasmoon
Copy link
Contributor

Hi @LukasTy @MBilalShafi ,

A thousand apologies, I had tested this during development but when yearsOrder was changed from a boolean to asc | desc the RTL horizontal direction check got borken.

Still works fine with the default settings but if yearsOrder='desc' is used with rtl now the left/right navigation is backwards when the theme is right to left.

Finally found a good place to test these under the localization docs page and this small change should make everything right (/x/react-date-pickers/adapters-locale/).

I will work on an update to the automated tests too that would catch this, but this quick fix for now.

@mui-bot
Copy link

mui-bot commented Sep 20, 2024

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

Generated by 🚫 dangerJS against 9317740

@zannager zannager added the component: pickers This is the name of the generic UI component, not the React module! label Sep 20, 2024
@flaviendelangle flaviendelangle added the bug 🐛 Something doesn't work label Sep 20, 2024
@flaviendelangle
Copy link
Member

Thanks for the quick fix !

I will work on an update to the automated tests too that would catch this, but this quick fix for now.

The next release is next Thursday, so we should be able to have 1-2 tests by then 👍

@LukasTy
Copy link
Member

LukasTy commented Sep 20, 2024

Thank you for the super quick fix! 🙏
I'm sorry that I did not spot this before approving. 🙈

As Flavien has said, by default, we would release at the end of next week.
This does not look like a critical bug and can probably wait for tests if you choose to add them. 👌

@thomasmoon
Copy link
Contributor Author

Thank you for the super quick fix! 🙏 I'm sorry that I did not spot this before approving. 🙈

As Flavien has said, by default, we would release at the end of next week. This does not look like a critical bug and can probably wait for tests if you choose to add them. 👌

I got the coverage added. The tests may not be the most beautiful, but the two cases of left and right in RTL mode definitely break in the previous version, so this could do the job! There weren't that many keyboard nav tests in the project to take a model from but this is surely a good addition. Happy to hear any feedback about the approach.

One interesting thing I ran into was that an additional Enter keyboard press would not trigger the onChange as expected, but it wasn't actually needed here because the focus change can be tested just by checking the active element. Now that I said that I realize I can remove that event from the tests.

@flaviendelangle flaviendelangle changed the title [pickers] Fix left-right keyboard nav with yearsOrder="desc" and isRTL [pickers] Fix left-right keyboard nav with yearsOrder="desc" and direction="rtl" Sep 25, 2024
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Looks good to me
Thanks for taking care of it!

@flaviendelangle flaviendelangle merged commit b899eb8 into mui:master Sep 25, 2024
18 checks passed
@LukasTy LukasTy changed the title [pickers] Fix left-right keyboard nav with yearsOrder="desc" and direction="rtl" [pickers] Fix keyboard navigation with yearsOrder="desc" and direction="rtl" Sep 30, 2024
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request Sep 30, 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants