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.

Sign up for GitHub

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

Jump to bottom

[Dialog] support rtl direction in dialog #32194

Closed
wants to merge 1 commit into from
Closed

[Dialog] support rtl direction in dialog #32194

wants to merge 1 commit into from

Conversation

kyeongsoosoo
Copy link

@kyeongsoosoo kyeongsoosoo commented Apr 8, 2022 •
edited

Closes #32022

Modal uses Portal, so It seems that dir attribute from root is not applied to Modal (includes Dialog), so I added a prop for Modal to check dir. If dir is rtl, dir attribute is passed to Root component in ModalUnstyled.
(I referred to the Slider component.)

Dialog uses Modal, so the bug is fixed.

Here is my Demo

@mui-bot
Copy link

mui-bot commented Apr 8, 2022 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against 09df3ea

@danilo-leal danilo-leal added the component: dialog This is the name of the generic UI component, not the React module! label Apr 8, 2022
oliviertassinari
oliviertassinari previously requested changes Apr 8, 2022
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited

Choose a reason for hiding this comment

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

I don't understand this PR. It seems to be about adding the dir attribute on the modal's root element but this should already be set on the body, this attribute inherits to all the children.

Screenshot 2022-04-08 at 17 22 24

https://mui.com/guides/right-to-left/#1-html

IMHO #32022 is a "behave as expected". Its reproduction is not following the instructions. The same reproduction following the instructions: https://codesandbox.io/s/stupefied-frost-o4zcd9?file=/src/index.js.

@kyeongsoosoo
Copy link
Author

@oliviertassinari Thanks for the information. If #32022 is not a bug or does not have to be fixed, I will close this PR.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2022 •
edited

Now, while I'm at it, I had a deeper dive, to see if we are not missing any opportunities. https://www.w3.org/International/questions/qa-html-dir suggests that <body dir="rtl"> is not right either, that it should be <html dir="rtl">, it's what Amazon does https://www.amazon.ae/?language=ar_AE. https://html.spec.whatwg.org/multipage/dom.html#dom-dir seems to point in the same direction. => I would propose we update the recommended element to apply the attribute on.

@kyeongsoosoo Let's wait @michaldudak inputs.

@michaldudak
Copy link
Member

What about cases where only part of the content of a page should be in RTL mode (a dictionary or translator app, for example). The dir attribute is valid on all elements, so shouldn't we support such a case?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2022 •
edited

What about cases where only part of the content of a page should be in RTL mode

@michaldudak Would <Modal><div dir="rtl"> be a good enough workaround? I mean, is the use case frequent enough to ask for saving one div, and adding more code for non-RTL users (98% of the market)?

@michaldudak
Copy link
Member

Even <Modal dir="rtl"> should do the trick. Assuming we update the docs and clearly state this caveat.

@oliviertassinari oliviertassinari mentioned this pull request Apr 8, 2022
Merged
1 task
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2022 •
edited

@michaldudak Even better 👌, so no implementation change is needed, there is already a native API. What I would suggest:

  1. We update https://mui.com/guides/right-to-left/#1-html to put <html> as the recommended element, <body> looks wrong.
  2. We update https://mui.com/guides/right-to-left/#1-html to remove the <div> demo. I think that I approved too quickly in [docs] Provide an alternative to right-to-left #25584, the diff of this PR feels wrong but the description of the PR is great. I would revert. Instead, we can cover 1. cases when accessing the <html> element is hard, then encourage developer to use the JS API in an useEffect 2. add a warning about dialogs that escape the normal dir attribute inheritance.
  3. Ship, wait and see if it's enough. If this topic comes up again, then assume that we did all we could with solving the problem at the docs level, and look into implementation changes now. It will cost to all the non-RTL users, which is the large majority but may be worth the tradeoff, e.g. implement [Dialog] Not supporting RTL direction from the theme #32022 (comment).

oliviertassinari
oliviertassinari reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited

Choose a reason for hiding this comment

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

.

@oliviertassinari oliviertassinari dismissed their stale review April 8, 2022 17:59

I unsubscribed to the thread, I gave a perspective on the matter, it's out of my hands now, I'm also removing the "request changes" review.

@kyeongsoosoo
Copy link
Author

@michaldudak If it is decided to update the docs and no implementation on code is needed, should I close this PR?

@michaldudak
Copy link
Member

@kyeongsoosoo yes, sorry for the confusion. I'll update the docs in another one.

@michaldudak michaldudak mentioned this pull request Apr 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

Assignees
No one assigned
Labels
component: dialog This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Dialog] Not supporting RTL direction from the theme
5 participants
@kyeongsoosoo @mui-bot @oliviertassinari @michaldudak @danilo-leal