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

[Tooltip] Fix arrow placement in RTL languages #18697

Closed
mosijava opened this issue Dec 5, 2019 · 2 comments · Fixed by #18706
Closed

[Tooltip] Fix arrow placement in RTL languages #18697

mosijava opened this issue Dec 5, 2019 · 2 comments · Fixed by #18706
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@mosijava
Copy link
Contributor

mosijava commented Dec 5, 2019

Tooltip's arrow placement in RTL languages is wrong.

Steps:

  1. change theme direction to 'rtl'
  2. set arrow property in a tooltip component

some thoughts to tackle the problem 🔦

in arrowGenerator() function in tooltip component if we add flip:false to it's JSS then I think the problem could be solved.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! labels Dec 5, 2019
@oliviertassinari
Copy link
Member

@mosijava Thank you for the bug report. It seems that it has been broken for a long time. It was already broken prior to the addition of the feature to the core:

Capture d’écran 2019-12-05 à 17 44 21

You are right, flip false seem to be enough (I also take care of the margin for account the border-radius:

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 1246561c5..d2ddc7ded 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -20,43 +20,59 @@ function round(value) {
 function arrowGenerator() {
   return {
     '&[x-placement*="bottom"] $arrow': {
+      flip: false,
       top: 0,
       left: 0,
       marginTop: '-0.95em',
+      marginLeft: 4,
+      marginRight: 4,
       width: '2em',
       height: '1em',
       '&::before': {
+        flip: false,
         borderWidth: '0 1em 1em 1em',
         borderColor: 'transparent transparent currentcolor transparent',
       },
     },
     '&[x-placement*="top"] $arrow': {
+      flip: false,
       bottom: 0,
       left: 0,
       marginBottom: '-0.95em',
+      marginLeft: 4,
+      marginRight: 4,
       width: '2em',
       height: '1em',
       '&::before': {
+        flip: false,
         borderWidth: '1em 1em 0 1em',
         borderColor: 'currentcolor transparent transparent transparent',
       },
     },
     '&[x-placement*="right"] $arrow': {
+      flip: false,
       left: 0,
       marginLeft: '-0.95em',
+      marginTop: 4,
+      marginBottom: 4,
       height: '2em',
       width: '1em',
       '&::before': {
+        flip: false,
         borderWidth: '1em 1em 1em 0',
         borderColor: 'transparent currentcolor transparent transparent',
       },
     },
     '&[x-placement*="left"] $arrow': {
+      flip: false,
       right: 0,
       marginRight: '-0.95em',
+      marginTop: 4,
+      marginBottom: 4,
       height: '2em',
       width: '1em',
       '&::before': {
+        flip: false,
         borderWidth: '1em 0 1em 1em',
         borderColor: 'transparent transparent transparent currentcolor',
       },

Capture d’écran 2019-12-05 à 18 04 18

Do you want to submit a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 5, 2019
@oliviertassinari oliviertassinari changed the title Tooltip's arrow placement in RTL languages [Tooltip] Fix arrow placement in RTL languages Dec 5, 2019
@mosijava
Copy link
Contributor Author

mosijava commented Dec 6, 2019

thanks a lot @oliviertassinari
yes it's an honor to do that, actually I have already done it ;)

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: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants