Full RTL support #1674

louy · 2015-09-17T17:58:06Z

Closes #1661.

louy · 2015-09-17T18:13:18Z

Travis build failed because of a network issue or something apparently. npm run build works fine.

shaurya947 · 2015-09-17T20:50:35Z

@louy maybe try pushing an empty commit to re-start the Travis build?

louy · 2015-09-17T21:06:17Z

@shaurya947 I believe admins can restart a build manually.
http://stackoverflow.com/a/17624403/1644422

It would be much cleaner than an empty commit.

louy · 2015-09-27T12:49:59Z

@shaurya947 can I get any feedback on this?

oliviertassinari · 2015-09-27T13:00:01Z

src/menu/menu-item.jsx

@@ -83,6 +83,14 @@ const MenuItem = React.createClass({
  },

  getStyles() {
+    const isRtl = this.context.muiTheme.isRtl;


I think that is should be this.state.muiTheme.isRtl

Fixed. This was written before state.muiTheme was introduced. I've just seen that feature.

louy · 2015-10-02T16:24:28Z

@oliviertassinari do you think adding another function to StylePropable that flips the direction automatically for all components might be a better approach?

oliviertassinari · 2015-10-02T16:32:48Z

Well, If you want to add the support of RTL on a large set of component, yes I would use a different approach.
I see two issues, first, we are going to duplicate this in a lot of different component. Second, in terms of perf/memory, I think that we can do better.
My idea would be to precompute on object called RTL at the theme level depending on the isRTL boolean.

shaurya947 · 2015-10-02T16:42:13Z

Sorry @louy, I've been a little MIA on this issue.

I don't think using the mixin is the best option. But I agree with @oliviertassinari that having something on the theme-level is a good approach. Take a look at theme-manager and see if you have any ideas :-)

oliviertassinari · 2015-10-02T17:11:31Z

For instance, when we contruct the theme we can compute a LTR variable that we add to the theme.

if (isRtl) {
  LTR = {
    right: 'left',
    left: 'right',
    marginRight: 'marginLeft',
    paddingLeft: 'paddingRight',
  }
} else {
  LTR = {
    right: 'right',
    left: 'left',
    marginRight: 'marginRight',
    paddingLeft: 'paddingLeft',
  }
}

shaurya947 · 2015-10-02T17:37:32Z

👍

…o rtl

louy · 2015-10-11T20:25:32Z

Hi,
I've made direction flipping automated using the function "prepareStyles" in style-propable. Please try to get back on this and merge it before making edits because it wasn't easy to do and took a lot of effort to refactor some stuff.

To see it in action, just uncomment the line in docs/src/components/master.jsx.

A few things to be careful about:

Never call prepareStyles on the same "style" object twice. You'll get a warning if you do so.
I wasn't able to reverse CircularProgress and LoadingIndicator yet.
Not all css properties are supported yet. We have padding, margin, border, transform, float, direction, right/left. Those are the ones I found that are being used in the project currently.
I should probably add unit tests for utils/styles. I just wanted to finish this asap to avoid rebasing.
I thought you might have some performance considerations. So when isRtl isn't set to true the ensureDirection function does nothing. This means performance won't be affected.

alitaheri · 2015-10-11T20:34:09Z

WOW, I love it 👍 👍 ❤️ ❤️

louy · 2015-10-11T20:43:55Z

I also wasn't able to add rtl support to all docs pages because they're written in es6 class style and I need to use mixins. That shouldn’t be a big issue.
The CodeMirror component has a tiny issue with spacing as well. It's not a full react component and I didn't get the chance to fix it yet. :)

oliviertassinari · 2015-10-11T20:54:59Z

src/utils/styles.js

+      }, style);
+    }
+
+    if (!muiTheme.isRtl) return style; // Performance hack!


Performance hack!

I would rather say Left to right is the default.
What do you think?

Left to right is the default doesn't imply that this line of code is there for optimization. Maybe:
Skip further processing to improve performance, since ltr is the default.

My point is that this line isn't for optimisation. It's there because the default style is for LTR. Hence, we shouldn't invert the style properties if isRtl is false.

oliviertassinari · 2015-10-11T21:26:29Z

I like this approach. I'm wondering if this can work with flexbox.
Aside from that. I think that it would be better to squash all the commits into one or two (for history readability).

louy · 2015-10-11T21:29:49Z

Awesome! It's working fine with <Grid> so I guess we won't be having issues.
There might be some possible cleanups (like changing mergeAndPrefix to just mergeStyles). I'll write some documentation about this whole thing this week and send another PR.
I'll squash them all in a minute.

louy · 2015-10-12T07:25:30Z

@shaurya947 can we get this merged soon?

shaurya947 · 2015-10-13T15:18:36Z

src/styles/raw-themes/dark-raw-theme.js

@@ -18,4 +18,5 @@ module.exports = {
    borderColor: ColorManipulator.fade(Colors.fullWhite, 0.3),
    disabledColor: ColorManipulator.fade(Colors.fullWhite, 0.3),
  },
+  isRtl: false,


I feel like this shouldn't be part of the rawTheme, semantically speaking.

shaurya947 · 2015-10-13T15:23:19Z

@louy see the comments above and let me know what you think.

Also, there's a lot of changes here. Can you make sure that nothing breaks when isRtl is set to false (the default value I suppose)?

Good job consolidating all this btw 👍

louy · 2015-10-13T15:45:33Z

Hi @shaurya947,

Just did some cleanup and the a quick look through the whole docs to make nothing is broken. Everything looks fine.
I've removed styleConstants from ThemeManager as well.

Thanks :)

shaurya947 · 2015-10-14T14:52:51Z

I'll go ahead and merge this then, and we can tackle any issues that come along the way. Thanks @louy

Full RTL support

oliviertassinari · 2015-10-14T17:47:29Z

src/utils/styles.js

+    return newStyle;
+  },
+
+  prepareStyles(muiTheme, ...styles) {


muiTheme is not passed as parameter when used.
I had to revert this commit in this PR #1882.

(for reference) This is not a bug.
See src/mixins/style-propable.js#L35-L37

Oh sorry, I overlooked this.

oliviertassinari · 2015-10-19T07:58:39Z

@andreychev Can you submit a PR to fix this ?

RTL support for menu item

18c60dc

louy closed this Sep 19, 2015

louy reopened this Sep 19, 2015

oliviertassinari reviewed Sep 27, 2015
View reviewed changes

Merge branch 'master' of https://github.com/callemall/material-ui int…

c2a756f

…o rtl

louy force-pushed the rtl branch from 8777bf9 to 791a2e1 Compare October 11, 2015 20:19

louy changed the title ~~RTL support for <MenuItem>~~ Full RTL support Oct 11, 2015

oliviertassinari reviewed Oct 11, 2015
View reviewed changes

Full RTL Support

5212f0d

louy force-pushed the rtl branch from f34bad7 to 5212f0d Compare October 11, 2015 21:36

shaurya947 reviewed Oct 13, 2015
View reviewed changes

louy added 2 commits October 13, 2015 18:32

Remove styleConstants from theme and isRtl from rawTheme

c045038

Remove a few auto-prefix calls

fe587a1

shaurya947 added a commit that referenced this pull request Oct 14, 2015

Merge pull request #1674 from louy/rtl

e59a0be

Full RTL support

shaurya947 merged commit e59a0be into mui:master Oct 14, 2015

oliviertassinari reviewed Oct 14, 2015
View reviewed changes

louy mentioned this pull request Oct 14, 2015

How To Override Some Styles That Are Not In Theme Variables #1355

Closed

louy deleted the rtl branch October 14, 2015 18:31

shaurya947 mentioned this pull request Oct 20, 2015

check muiTheme is defined #1935

Closed

zannager added the component: menu This is the name of the generic UI component, not the React module! label Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full RTL support #1674

Full RTL support #1674

louy commented Sep 17, 2015

louy commented Sep 17, 2015

shaurya947 commented Sep 17, 2015

louy commented Sep 17, 2015

louy commented Sep 27, 2015

oliviertassinari Sep 27, 2015

louy Sep 27, 2015

louy commented Oct 2, 2015

oliviertassinari commented Oct 2, 2015

shaurya947 commented Oct 2, 2015

oliviertassinari commented Oct 2, 2015

shaurya947 commented Oct 2, 2015

louy commented Oct 11, 2015

alitaheri commented Oct 11, 2015

louy commented Oct 11, 2015

oliviertassinari Oct 11, 2015

alitaheri Oct 11, 2015

oliviertassinari Oct 11, 2015

louy Oct 11, 2015

oliviertassinari commented Oct 11, 2015

louy commented Oct 11, 2015

louy commented Oct 12, 2015

shaurya947 Oct 13, 2015

shaurya947 commented Oct 13, 2015

louy commented Oct 13, 2015

shaurya947 commented Oct 14, 2015

oliviertassinari Oct 14, 2015

louy Oct 14, 2015

oliviertassinari Oct 14, 2015

oliviertassinari commented Oct 19, 2015

Full RTL support #1674

Full RTL support #1674

Conversation

louy commented Sep 17, 2015

louy commented Sep 17, 2015

shaurya947 commented Sep 17, 2015

louy commented Sep 17, 2015

louy commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louy commented Oct 2, 2015

oliviertassinari commented Oct 2, 2015

shaurya947 commented Oct 2, 2015

oliviertassinari commented Oct 2, 2015

shaurya947 commented Oct 2, 2015

louy commented Oct 11, 2015

alitaheri commented Oct 11, 2015

louy commented Oct 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Oct 11, 2015

louy commented Oct 11, 2015

louy commented Oct 12, 2015

Choose a reason for hiding this comment

shaurya947 commented Oct 13, 2015

louy commented Oct 13, 2015

shaurya947 commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Oct 19, 2015