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

[docs] RTL support #1926

Closed
AdamGorkoz opened this issue Oct 19, 2015 · 46 comments
Closed

[docs] RTL support #1926

AdamGorkoz opened this issue Oct 19, 2015 · 46 comments
Labels
docs Improvements or additions to the documentation

Comments

@AdamGorkoz
Copy link
Contributor

Is there no way to set all the components in the application to be RTL ? Can anyone give an accurate example of how to do this ?

@oliviertassinari
Copy link
Member

@louy Should be able to help you.

@louy
Copy link
Contributor

louy commented Oct 19, 2015

Yep. See here.

In your root component:

  getInitialState () {
    const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
    // To switch to RTL...
    muiTheme.isRtl = true;
    return {
      muiTheme,
    };
  },

@AdamGorkoz
Copy link
Contributor Author

And how would you do it if it was es6 ? I dont know if you had experience with it but you have a constructor for components instead of getInitialState in ES6 , Doesn't seem logical to return an object from a constructor

@louy
Copy link
Contributor

louy commented Oct 19, 2015

I think it would be something like this:

constructor() {
    const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
    // To switch to RTL...
    muiTheme.isRtl = true;
    this.state = { muiTheme };
}

Take a look here for more info.

@AdamGorkoz
Copy link
Contributor Author

This doesn't seem to work , What am i missing ? I'd really appreciate it if you took a look.

"use strict";

import React from "react";

const { AppBar,
    AppCanvas,
    FontIcon,
    IconButton,
    EnhancedButton,
    Menu,
    Mixins,
    RaisedButton,
    Styles,
    Tab,
    Tabs,
    Paper} = require('material-ui');

const DefaultRawTheme = Styles.LightRawTheme;

const ThemeManager = require('material-ui/lib/styles/theme-manager');

var  App = React.createClass({
    getInitialState: function(){
        const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
        // To switch to RTL...
        muiTheme.isRtl = true;
        return {
            muiTheme,
        };

    },
    render(){
       return (
         <div>
             <AppBar
                 title="Adiga Mundial"
                 iconClassNameRight="muidocs-icon-navigation-expand-more" />
             <div className="container">
                 <Paper zDepth={2}>
                     <p>zDepth=1</p>
                 </Paper>
                {this.props.children}
             </div>
         </div>
       );
   }
});

export default App;

@louy
Copy link
Contributor

louy commented Oct 19, 2015

You're missing the childContext part. Just add those things.

Sent from my iPhone

On 20 Oct 2015, at 00:37, turka50 notifications@github.com wrote:

This doesn't seem to work , What am i missing ? I'd really appreciate it if you took a look.

"use strict";

import React from "react";

const { AppBar,
AppCanvas,
FontIcon,
IconButton,
EnhancedButton,
Menu,
Mixins,
RaisedButton,
Styles,
Tab,
Tabs,
Paper} = require('material-ui');

const DefaultRawTheme = Styles.LightRawTheme;

const ThemeManager = require('material-ui/lib/styles/theme-manager');

var App = React.createClass({
getInitialState: function(){
const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
// To switch to RTL...
muiTheme.isRtl = true;
return {
muiTheme,
};

},
render(){
   return (
     <div>
         <AppBar
             title="Adiga Mundial"
             iconClassNameRight="muidocs-icon-navigation-expand-more" />
         <div className="container">
             <Paper zDepth={2} isRtl={true}>
                 <p>zDepth=1</p>
             </Paper>
            {this.props.children}
         </div>
     </div>
   );

}
});

export default App;

Reply to this email directly or view it on GitHub.

@AdamGorkoz
Copy link
Contributor Author

Well now it moved the app-bar icon to the right a bit but the title didnt... not exactly RTL yet . Wondering if this component is bugged.

@louy
Copy link
Contributor

louy commented Oct 20, 2015

Hmm. I think you should add direction="rtl" to your html file.
Here's how it should look like:
screen shot 2015-10-20 at 10 09 01

As opposed to the LTR docs:
screen shot 2015-10-20 at 10 09 06

@louy
Copy link
Contributor

louy commented Oct 20, 2015

You know, instead of using <div> as the root element, use <AppCanvas>. It should solve your problem.

@AdamGorkoz
Copy link
Contributor Author

What is the AppCanvas component , what does it do ? I don't see it in the components list.

@louy
Copy link
Contributor

louy commented Oct 20, 2015

I believe it's a component that's supposed to wrap your whole application. Adds background color etc...

I'm not sure why it's not documented though.
@oliviertassinari @shaurya947 any comments on this?

@oliviertassinari
Copy link
Member

AppCanvas is an undocumented component to do some layout with an AppBar. It's useful for displaying the AppBar at the top of the page, in a fixed position. I would not use it if I were you.

@oliviertassinari
Copy link
Member

I'm using a custom component to acheive the same goal as this one as some limitations

@shaurya947
Copy link
Contributor

@louy @oliviertassinari after seeing this issue, I think it would be a good idea to add a section to the docs site for RTL. @louy, would you like to take the lead on this?

@louy
Copy link
Contributor

louy commented Oct 20, 2015

@oliviertassinari Hmm. I'd say let's add an option to <AppCanvas> for the fixed <AppBar> and make it public. Then RTL would be a piece of cake. I just need a parent element to have the "direction" style property.

@TURKA50 the alternative solution for now is adding direction="rtl" to html tag. that should be it.

@shaurya947 yes sure. I'll try to do it over the weekend.

@oliviertassinari
Copy link
Member

@louy, I would not use AppCanvas because it's undocumented. However, I agree, having a good layouting component for the AppBar and the content below is valuable.

@AdamGorkoz
Copy link
Contributor Author

@louy You were right , adding rtl to the html tag solved the problem. Though i couldn't get this to work with the ES6 syntax , some issue with the childContextType , I resorted to using ES5 for this component.

Thanks.

@louy
Copy link
Contributor

louy commented Oct 20, 2015

I'm not really sure how to setup context in ES6. Sorry about that.
You're welcome.

@AdamGorkoz
Copy link
Contributor Author

I just noticed a very confusing thing , when you switch to rtl and you use AppBar it has the events onLeftIconButtonTouchTap and onRightIconButtonTouchTap , So these are not actually right anymore they are the exact opposite.

For your consideration.

@louy
Copy link
Contributor

louy commented Oct 20, 2015

Yep they are. Also all "right" and "left" icons are switched but their names are not.
This is only to make bidirectional apps easier to maintain.

@alitaheri alitaheri added this to the Improve the documentation milestone Dec 6, 2015
@alitaheri alitaheri added docs Improvements or additions to the documentation Core labels Dec 9, 2015
@oliviertassinari oliviertassinari removed this from the Improve the documentation milestone Feb 1, 2016
@skariel
Copy link

skariel commented Mar 3, 2016

hi, I'm having some problem with this too -- isRtl doesn't seem to work right. Using material-ui@0.15.0-alpha.1 this is the minimal code to reproduce:

import React from 'react'
import ReactDOM from 'react-dom'
import injectTapEventPlugin from 'react-tap-event-plugin'

import MuiThemeProvider from 'material-ui/lib/MuiThemeProvider';
import getMuiTheme from 'material-ui/lib/styles/getMuiTheme';

import AppBar from 'material-ui/lib/app-bar';

const muiTheme = getMuiTheme({isRtl:true})

const Main = class extends React.Component {
    render() {
        return (
            <MuiThemeProvider muiTheme={muiTheme}>
                <AppBar title="MyApp" />
            </MuiThemeProvider>
        );
    }
}

injectTapEventPlugin();
ReactDOM.render( <Main /> , document.getElementById('app'))

and the result:

screenshot from 2016-03-03 23 46 22

setting isRtl:false looks normal:

screenshot from 2016-03-03 23 48 37

also adding a direction='rtl' to the html tag doesn't help. Neither helps AppCanvas, or downgrading to 0.14.4

Am I doing something wrong? Thanks!

@louy
Copy link
Contributor

louy commented Mar 4, 2016

@skariel can you try previous versions and see if it's something that got broken recently? it was working in 0.12.4+.

@skariel
Copy link

skariel commented Mar 5, 2016

@louy using material-ui@0.12.5 and the following code results in problems as described below:

import React from 'react';
import {AppBar, Styles} from 'material-ui';
import ThemeManager from 'material-ui/lib/styles/theme-manager';

const DefaultRawTheme = Styles.LightRawTheme;

var  Main = React.createClass({
    getInitialState: function(){
        const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
        muiTheme.isRtl = true;
        return {
            muiTheme,
        };

    },
    getChildContext () {
      return {
        muiTheme: this.state.muiTheme,
      };
    },
    render(){
       return (
         <AppBar
             title="Adiga Mundial"
             iconClassNameRight="muidocs-icon-navigation-expand-more" />
       );
   }
});

React.render( <Main /> , document.getElementById('app'))

this code as-is, doesn't present anything in the page. Only way to make it display the AppBar is to comment out the getChildContext above. It doesn't display RTL.

using material-ui@0.13.0 the code above does not compile, giving the following error:

ERROR in ./~/material-ui/lib/styles/theme-manager.js
Module not found: Error: Cannot resolve module 'react-addons-update' in /home/skariel/Desktop/sapakim/node_modules/material-ui/lib/styles
 @ ./~/material-ui/lib/styles/theme-manager.js 6:13-43

Also 0.14.x behavior is different from 0.15.0-alpha for the first code I showed, in 0.15 isRtl=true actually propagates to children, in 0.14.x it doesn't.

So I'm new to this lib, maybe I'm doing something wrong? I read the theming docs for the correct version...

EDIT: rtl to to true in code block above

@skariel
Copy link

skariel commented Mar 5, 2016

OK -- I was missing the contextTypes and childContextTypes methods. Made it work in 0.12.5 with the code below. Still would be nice to achieve the same with 0.15.0-alpha. Will continue trying

import React from 'react';
import {AppBar, AppCanvas, Styles} from 'material-ui';
import ThemeManager from 'material-ui/lib/styles/theme-manager';

const DefaultRawTheme = Styles.LightRawTheme;

var  Main = React.createClass({
    getInitialState: function(){
        const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
        muiTheme.isRtl = true;
        return {muiTheme};

    },
    contextTypes: {
       muiTheme: React.PropTypes.object,
     },

     childContextTypes: {
       muiTheme: React.PropTypes.object,
     },
    getChildContext () {
      return {
        muiTheme: this.state.muiTheme,
      };
    },
    render(){
       return (
           <AppCanvas>
         <AppBar
             title="Adiga Mundial"
             iconClassNameRight="muidocs-icon-navigation-expand-more" />
     </AppCanvas>
       );
   }
});

React.render( <Main /> , document.getElementById('app'))

@skariel
Copy link

skariel commented Mar 5, 2016

Some progress! the code below is broken only by 0.14.4 and works with all lower versions. I'm currently checking for changes in version that could have done this... maybe will do a PR to fix this:

import React from 'react';
import ReactDOM from 'react-dom';
import {AppBar, AppCanvas, Styles} from 'material-ui';
import ThemeManager from 'material-ui/lib/styles/theme-manager';

const DefaultRawTheme = Styles.LightRawTheme;

var  Main = React.createClass({
    getInitialState: function(){
        const muiTheme = ThemeManager.getMuiTheme(DefaultRawTheme);
        muiTheme.isRtl = true;
        return {muiTheme};
    },
    contextTypes: {
       muiTheme: React.PropTypes.object,
     },
     childContextTypes: {
       muiTheme: React.PropTypes.object,
     },
    getChildContext () {
      return {
        muiTheme: this.state.muiTheme,
      };
    },
    render(){
       return (
           <AppCanvas>
         <AppBar
             title="Adiga Mundial"
             iconClassNameRight="muidocs-icon-navigation-expand-more" />
     </AppCanvas>
       );
   }
});

ReactDOM.render( <Main /> , document.getElementById('app'))

@skariel
Copy link

skariel commented Mar 5, 2016

@mbrookes doing this binary search manually :)

the problematic commit is 2f0d184

reverting changes made to src/mixins/style-propable.js solves the problem. Judging by the header in that file this should not break other stuff

so,

doing a PR now

@skariel
Copy link

skariel commented Mar 5, 2016

actually the fix above works for 0.14.4 but not for master... so theres another breaking commit somewhere

back to searching then

@mbrookes
Copy link
Member

mbrookes commented Mar 5, 2016

Nice detective work @skariel!

@oliviertassinari
Copy link
Member

Ohhh. We should add some unit test for this feature.

@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

@skariel You might be underestimating the difficulty of reverting that commit 😄.

I just tested RTL support locally on the docs site and it's working fine. You have to override isRtl to true AND set direction: 'rtl' in the css. Are you sure that's not working? (I'm using master but I can try any commit version you're working with to be sure).

@skariel
Copy link

skariel commented Mar 5, 2016

no -- I didn't try with both. Will try now, sorry for the noise if this is the problem

@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

@skariel Sorry for the late response on this. I've worked on some of this code so should have been more responsive. Just ping me if you're having any issues and I'll help get this resolved ASAP.

@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

One additional note, mutating muiTheme directly will definitely cause problems for RTL. Your particular code example using the alpha wasn't doing that, but there are code examples from others or in different versions where that is happening and will no longer work.

  getInitialState () {
    const muiTheme = getMuiTheme();
    muiTheme.isRtl = true; // WRONG do getMuiTheme({isRtl: true}) instead
    return {
      muiTheme,
    };
  },

@skariel
Copy link

skariel commented Mar 5, 2016

@newoga yep, it works, sorry for the noise and thanks for the support. Maybe worth writing a bit more about this in the docs I don't see there direction is discussed. I did try it after reading this thread but never with the code with the alpha above

thanks again, great library, hopefully next time I encounter a real bug so I can fix it ;)

@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

thanks again, great library, hopefully next time I encounter a real bug so I can fix it ;)

Thank you @skariel, I really appreciate you digging into this. You're welcome anytime 😄 And yes, we should provide an example for RTL 👍

@newoga newoga changed the title RTL support for all components [Docs] RTL support Mar 5, 2016
@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

@skariel Any chance you have some free time to submit a PR that improves the documentation on how to do this properly?

At some point, it might be cool to toggle direction in the themes customization section as well. Let me know!

@skariel
Copy link

skariel commented Mar 5, 2016

@newoga Sure, I have some time next week

@newoga
Copy link
Contributor

newoga commented Mar 5, 2016

Awesome, thanks much @skariel!

@newoga newoga removed the Regression label Mar 9, 2016
@mbrookes
Copy link
Member

mbrookes commented Apr 2, 2016

Ping @skariel 😄

@skariel
Copy link

skariel commented Apr 2, 2016

hi, I'm here. The thing is I'm no longer using material-ui because I couldn't configure it with react as an external library using webpack. This made bundle.js way too big (added ~500Kb). The result is that I'm using material design lite. I don't have a working material-ui configuration anymore, I'm learning this other tool, so I won't be able to help with docs... I should have notified you when I decided this. Sorry for the delay ...

@mbrookes
Copy link
Member

mbrookes commented Apr 2, 2016

Hi @skariel - thanks for letting us know. I'm sure someone else this is important to will help out. 👍

@agent3bood
Copy link

here is how I gor rtl working
1 add dir to your html<html dir="rtl">
2 in your main component do this <MuiThemeProvider muiTheme={getMuiTheme({isRtl:true})}>

make sure to import import getMuiTheme from 'material-ui/styles/getMuiTheme'

@YochayCO
Copy link

YochayCO commented Dec 8, 2016

Hi guys! I got the rtl to work, mostly... but I have tried using Tabs and it seems the animation stops working only when I set isRtl to true...
Not sure yet if this happens on other components as well.

@oliviertassinari oliviertassinari changed the title [Docs] RTL support [docs] RTL support Jul 30, 2017
@oliviertassinari
Copy link
Member

It's officially supported with the v1-beta version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

No branches or pull requests

10 participants