Skip to content

WIP: Add RTL support#11931

Closed
mdo wants to merge 1 commit intomasterfrom
rtl
Closed

WIP: Add RTL support#11931
mdo wants to merge 1 commit intomasterfrom
rtl

Conversation

@mdo
Copy link
Copy Markdown
Member

@mdo mdo commented Dec 19, 2013

This is a heavily WIP pull request to add RTL support to v3.1.

Strategy

There are two primary ways to implement RTL: rewrite the core files to include mixins for directional properties, or a separate optional file. I've opted for the latter here.

Implementation

This stubs out the ground work for adding RTL by updating the Gruntfile to compile and minify the CSS files for bootstrap-rtl.css from rtl.less. It also adds an example, based on the Theme example, to showcase the changes. Currently it looks like this:

screen shot 2013-12-18 at 8 40 04 pm
screen shot 2013-12-18 at 8 40 42 pm

Feedback

I'm pausing here to get feedback from folks. This results in a lot of duplicate code, which sucks in some ways and doesn't in others. I'd love for folks to weigh in on things to be aware of, alternate implementations, bugs in what I've done thus far, oversights, etc.

@cvrebert
Copy link
Copy Markdown
Collaborator

Relevant outstanding PRs: #10281, #10672
Relevant open issue: #11134

This was referenced Dec 19, 2013
@danielkatz
Copy link
Copy Markdown

I'm very glad that finally you considering including RTL support! I've brought it up several months ago, and i want to repeat myself a bit.
Back than, I suggested avoiding all the duplication in the core LESS code, by simply employing some sort of LESS technique to generate 2 different CSS (say: bootstrap.css, and bootstrap.rtl.css) files from single LESS codebase.
Consider something like that: .class { .float(start); } which will be compiled to .class { float: left; } when the value of @bidi in the current scope is ltr and to .class { float: right; } when the value is rtl. There are several fully implemented open-source approaches to this technique including my own less-bidi.

Another thing to keep in mind is that some of the js components of twbs are also not direction-agnostic and need to be adopted to support RTL.

@uda
Copy link
Copy Markdown

uda commented Dec 19, 2013

+1
A separate file provides a better solution for multilingual websites.

@morteza
Copy link
Copy Markdown

morteza commented Dec 19, 2013

Making things RTL is not a new problem where right-to-left scripts are used. When you were working on Bootstrap v2, I always wanted something like theme capability, that you can change whatever you want (to some degree) without changing the core (the reference).

The best method so far, seems to be the theme solution. You've added theming capability without any actual use. RTL will be one of the first real world application for it. Right-to-left is nothing more than representation, and adding mixins or variables is nonsense to me. Core bootstrap has to be clean and minimal, and semantically consistent. RTL is not semantic, and core module of BT3 has to be concerned about semantic, not different representations of a component.

Let me predict what will happen in the future if you add RTL support to the core by variables and mixins (and not as a theme): It will continue the way Semantic-UI and Foundation went: No consistency and sometime different representations from the original bootstrap.

So here is what I believe is the best practice: Close this WIP, and leave it to the projects like https://github.com/morteza/bootstrap-rtl or similar ones, where they develop a theme on top of Bootstrap 3, to make it RTL. When they need RTL, its CSS will be added like a regular theme. Otherwise it will be removed. No big deal. That's it.

The separate file solution is the best answer to the issue, and including it in the core less files would be redundant. It's just another representation! Nothing more. It has to be removed completely from the core, and it seems enough that you reference some of the themes developed for this capability.

@morteza
Copy link
Copy Markdown

morteza commented Dec 19, 2013

Here are some issues I've faced when developing RTL theme for BT3:

  1. DOM arrangements are important, and different from LTR. Examples are: icon before text or after it in buttons, next month and last month buttons in calendars and date pickers, ...
  2. pull-right and pull-left both have different meaning and representation from their original counterpart in bootstrap. Just change them to more appropriate classes for both RTL and LTR (BIDI).
  3. Next and Prev buttons of paginations have to be replaces with each other in RTL.
  4. I had to change component orders in the examples directory (in HTML), to make them real RTL.
  5. It's not something like replacing all lefts with rights and vice versa.

Long story short, in long term, the Bootstrap project has to move toward BIDI solutions like what happened in operating systems world. Bidirectional texts and representations seems more fit than RTL. RTL is just a theme, nothing more.

Cheers.

@daniel-white
Copy link
Copy Markdown

We are using our enhanced version of R2. It makes things less error prone than maintaining separate files. Consider code generating the RTL stuff at build time.

@KingYes
Copy link
Copy Markdown

KingYes commented Dec 19, 2013

Nice work guys.
I think the better way its use with a separate file.

@hero-m
Copy link
Copy Markdown
Contributor

hero-m commented Dec 19, 2013

I have to say, I'm really in favor of the current strategy for adding RTL support (a separate override file).
I understand that there are many different practices and requirements for adding RTL support to websites, but this is the only one that can meet all the requirements with a few basic lines in the less files. for example, one can easily merge and prefix the RTL code like this:

@include "bootstrap.less"
[dir="rtl"] {
  @include "rtl.less"
}

An important requirement and argument against using mixins is having content of different languages in the same page (for example, an english website containing an article in arabic). This can easily be supported using a prefixed RTL version like the one mentioned above, but would be impossible to do using mixins. I believe such a use-case will become more common as more websites support multilingual content or accessibility standards such as "Language of Parts" become widespread.

@OlivierJaquemet
Copy link
Copy Markdown

Please, whatever solution you choose, make sure to allow both ltr and rtl direction in the same page.
Those are perfectly legitimate use cases, for example in a page whose default language and direction are english / left to right (with attributes dir="ltr" lang="en" on html tag), but in which an abstract of an arabic text in right to left has been quoted (with the attributes dir="rtl" lang="ar" on wrapper element).

@KingYes
Copy link
Copy Markdown

KingYes commented Dec 19, 2013

@OlivierJaquemet, are you try flip theme from ltr to rtl?

@smartcorestudio
Copy link
Copy Markdown

+1 for the separate file

@modusinternet
Copy link
Copy Markdown

I totally agree with the points both 'hero-m' and 'OlivierJaquemet' described above. I am the lead designer of a multilingual CMS, https://github.com/modusinternet/custodian-cms, which allows administrators to add content to their website in any language.

i.e. If a visitors language preferences are set to Hindi but the default language of the website is English, so long as the admin has added accompanying Hindi versions of the English content, then Hindi will appear. In areas where no Hindi translation is yet available then the default English will appear.

This means I may have up to 2 languages dynamically appearing on the same page which use different directional properties.

@uda
Copy link
Copy Markdown

uda commented Dec 23, 2013

@morteza, I believe you have a point, yet I think the best approach is to change the core, if not for in-core support then at least for more semantic mixins, like instead of using pull-right and pull-left it should be pull-start and pull-end.
From that point, I think @danielkatz got it perfect in his BIDI solution, since the main difference between RTL and LTR is the starting point, all the rest is relatively the same.

@okhayat
Copy link
Copy Markdown

okhayat commented Dec 23, 2013

+1 for the separate file
Use pull-start and pull-end as pull-{right,left} are confusing with RTL
Use the same value of 'left' for 'right' when using 'absolute' position whenever possible
Set one main 'font-family' declaration for elements with similar font faces to make it easier to use suitable RTL fonts

@hero-m
Copy link
Copy Markdown
Contributor

hero-m commented Dec 23, 2013

I don't think this debate on using pull-start|end vs pull-right|left is productive at all. Any themer can define a pull-start|end class in his theme and get it done right in seconds. This is not the issue. The issue is when a themer uses pull-right|left in his theme and we want to use the theme in an RTL page, the layout shouldn't break. it should be consistent. and the current overrides on pull-right|left as suggested by @mdo fixes this issue.
And I believe this is so awesome to have RTL support in bootstrap itself, that we shouldn't block progress here for these minor issues. I don't want to have to search at the bottom of forums or blog posts for the right RTL solution, or fear that the next upgrade might break my RTL theme anymore ;)

@okhayat
Copy link
Copy Markdown

okhayat commented Dec 23, 2013

Sure. Just ignore that start|end thing for now ;)

@morteza
Copy link
Copy Markdown

morteza commented Dec 23, 2013

I agree with @hero-m's comment. And I completely disagree with calling them RTL and LTR. BIDI seems more appropriate for what we actually looking for. So here is a possible solution:

  1. Single or multiple LESS file for BIDI, separated from the core (rtl.less or bidi.less?). Consider that we might need some more LESS files, so a directory structure maybe?
  2. Another "Optional" target for Grunt, to make single bootstrap-rtl css file. Or merge the rtl into the bootstrap's own css file? (which is wrong and inconsistent with current design of bootstrap). Best practice here, is do something we already do for theme.less: Build and test it by default as a separate target file. As a result, we don't need to change any example or production codes from LTR to RTL.

I've already done it initially, and it seems clean in design, and easy to deploy. I don't know...

@itaymesh
Copy link
Copy Markdown

I have been using using bi-app
lesshttp://anasnakawa.github.io/bi-app-less/and it worked great for
me so far.
I also have bootstrap 3 less files already implementing its mixins.
I will be happy to share it....

On Thu, Dec 19, 2013 at 6:42 AM, Mark Otto notifications@github.com wrote:

This is a heavily WIP pull request to add RTL support to v3.1.
Strategy

There are two primary ways to implement RTL: rewrite the core files to
include mixins for directional properties, or a separate optional file.
I've opted for the latter here.
Implementation

This stubs out the ground work for adding RTL by updating the Gruntfile to
compile and minify the CSS files for bootstrap-rtl.css from rtl.less. It
also adds an example, based on the Theme example, to showcase the changes.
Currently it looks like this:

[image: screen shot 2013-12-18 at 8 40 04 pm]https://f.cloud.github.com/assets/98681/1780199/b0876080-6867-11e3-8baa-f3032b9403fe.png
[image: screen shot 2013-12-18 at 8 40 42 pm]https://f.cloud.github.com/assets/98681/1780200/c0a516e2-6867-11e3-8691-7bd4b53301cc.png
Feedback

I'm pausing here to get feedback from folks. This results in a lot of
duplicate code, which sucks in some ways and doesn't in others. I'd love
for folks to weigh in on things to be aware of, alternate implementations,

bugs in what I've done thus far, oversights, etc.

You can merge this Pull Request by running

git pull https://github.com/twbs/bootstrap rtl

Or view, comment on, or merge it at:

#11931
Commit Summary

  • stub out a few rtl things

File Changes

Patch Links:

איתי רז

0544.635.797
03.5664254
studioraz.co.il

@mdo
Copy link
Copy Markdown
Member Author

mdo commented Dec 24, 2013

Punting on this implementation for the time being.

@mdo mdo closed this Dec 24, 2013
@mdo mdo deleted the rtl branch December 24, 2013 03:04
@elkebirmed
Copy link
Copy Markdown

Can you tell me about the result of this WIP?
Solved or not yet!?

@cvrebert
Copy link
Copy Markdown
Collaborator

cvrebert commented May 3, 2014

Successor: #12840.

@itaymesh
Copy link
Copy Markdown

itaymesh commented May 3, 2014

I have been using https://github.com/twitter/css-flip or
https://github.com/itaymesh/gulp-css-flip for this task and it works great.

On Sat, May 3, 2014 at 11:30 AM, Chris Rebert notifications@github.comwrote:

Successor: #12840 #12840.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11931#issuecomment-42099471
.

איתי רז

0544.635.797
03.5664254
studioraz.co.il

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.