users/adamsiv/rtl-support #27027

DigitalKrony · 2018-08-06T20:19:53Z

On behalf of Microsoft, I'm submitting this pull-request.

We have taken Bootstrap 4.x on as a dependency for a project, however, we have requirements for our websites to work in an international arena as well as maintain perf goals and an easy dev story. To that end, the best case in order to align these things is to integrate internationalization directly into Bootstrap. You'll find the appropriate update within.

The chages in the files below simply add in the dir attribute to the HTML tag to allow ease of testing. While testing the RTL story wihtin the doc site, these must be set to "rtl".

site > _layouts >
- default.html
- docs.html
- home.html
- redirect.html

The update to the SCSS is realativly simple. I've added three variables to the _variables.scss.

$dir:String = ltr (default) || rtl
$left:String = left (default) || right (is set based on $dir)
$right:String = right (default) || left (is set based on $dir)

These three variables work in conjunction with eachother. By !default $dir is set to ltr. Following this varaible, there's $left and $right. These two variables are set via a function (set-dir(left|right) ) base on the value of $dir for ease of use. The $dir variable also sets a global direction: $dir style decloration to capture finer style requirements for things like inputs and sudo elements.

wolfy1339 · 2018-08-06T21:15:01Z

This is a duplicate, there is already a PR that deals with LTR and RTL functionality

…lKrony/bootstrap into users/adamsiv/rtl-support

wolfy1339 · 2018-08-07T03:51:19Z

Please don't commit the dist files as per the Contribution guidelines of this repository

XhmikosR · 2018-08-07T10:53:06Z

/CC @mdo: we really need to look into RTL support...

XhmikosR · 2018-08-07T16:48:43Z

@DigitalKrony: there are still dist files here (js/dist)

…lKrony/bootstrap into users/adamsiv/rtl-support

XhmikosR · 2018-08-07T16:54:03Z

There are still more dist files like the docs CSS and https://github.com/twbs/bootstrap/pull/27027/files#diff-2f2d399f0ea8844859fe5514b304733b

XhmikosR · 2018-08-07T16:55:53Z

js/src/util.js

@@ -67,6 +67,13 @@ const Util = (($) => {

    TRANSITION_END: 'bsTransitionEnd',

+    getPageDirection() {
+      const htmlElement = document.getElementsByTagName('html')[0]


any reason why you don't use document.documentElement here?

I was aiming for coverage and the dev docs around documentElement seemed unsure of IE and Edge support. If there's info surrounding coverage, I'd rather use it over the current implementation and would update accordingly.

You work for Microsoft, you should be able to learn about it from the inside :P

But joking aside, works fine for me on latest Edge that ships with 10 stable and IE 11 and IE 10 (emulated).

Those worked for me as well. And Emulated doesn't always do the trick. #trustMe.

However, I ran it through some rough and quick tests on Browser Stack, seemed to work as expected. At least on Win10. If someone else wants to roll back to win7 and Vista and test it, please send me your findings. Making the update now.

…lKrony/bootstrap into users/adamsiv/rtl-support

andresgalante · 2018-08-08T13:04:44Z

Thanks a lot @DigitalKrony, as @wolfy1339 mentions there are other rtl PRs, including this one open:
#26722

@mdo, I agree with @XhmikosR we do need to address rtl. If we are following this solution I'd rather use #{start} and #{end} instead of #{left} and #{right} following the standard for CSS logical propierties.

DigitalKrony · 2018-08-08T17:15:08Z

Thanks, @andresgalante, for dropping in.

Were we consolidating into one PR or am I updating this one to align to guidance? Happy to do either. Just hoping to get this landed sooner rather than later.

andresgalante · 2018-08-09T14:04:17Z

@DigitalKrony it'll be up to @mdo to decide if this goes in and when. The thing to consider is that if this is creating a breaking change, it'll have to land in v5.

DigitalKrony · 2018-08-29T00:10:42Z

So @mdo and @XhmikosR, what do we need to do to get this in? I would really appreciate a look-see (other than resolving the conflicts that have worked their way in).

# Conflicts: # scss/_carousel.scss # scss/mixins/_grid-framework.scss # scss/mixins/_grid.scss # site/_layouts/redirect.html

XhmikosR · 2018-08-29T18:54:31Z

Unfortunately, it's not my decision to make. I know for sure this should have been addressed a long time ago for sure. For reference https://github.com/twbs/bootstrap/pulls?utf8=%E2%9C%93&q=is%3Apr+rtl

MartijnCuppens · 2018-09-03T17:17:39Z

RTL support would indeed be nice! I'm not familiar with making rtl websites, but was curious how this was done, so I have set up this PR locally to check some things.

I think in some cases the changes don't really change anything. Like here:

bootstrap/scss/_badge.scss

Lines 34 to 35 in c424555

    
           padding-#{$end}: $badge-pill-padding-x; 
        
           padding-#{$start}: $badge-pill-padding-x;

or here:

bootstrap/scss/_card.scss

Lines 108 to 109 in c424555

    
           margin-#{$end}: -($card-spacer-x / 2); 
        
           margin-#{$start}: -($card-spacer-x / 2);

and some other places.

About the carousel, I don't know if this is the behaviour we want:

And some dropdowns are broken:

@DigitalKrony, I assume you did a find-and-replace of the right and left appearances, combined with some extra tweaks but I think every component will need to be reviewed if we want a solid solution for rtl support.

DigitalKrony · 2018-09-04T17:41:48Z

@MartijnCuppens - I actually didn't do a find replace, I went file by file and corrected the issues I saw for each component by hand. However, this has been sitting, waiting to merge in for such a long time, that errors have started to work their way in. The screenshots that you've posted above, I remember having to massage out of the code when I first did my integration.

I'll re-merge and do the work again.

DigitalKrony · 2018-09-04T22:31:33Z

So I've gone back and reviewed the branch for the comments and inconsistencies mentioned above.

For the _badge.scs and _card.scss edits, I was aiming for consistency with regards to how padding and margin was handled. While, no, it's not really value after the files compile, it is adding value from a consistency standpoint as well as informing other on a proper way to utilize it in other places. It's also not hurting anything having these as variables.
Conserning the carousel, the glyphs for next and previous are varaible sets. However, I would agree that if a user built BS out of the box, it should look as expected. Those chevrons are no exception. So, I've added in a funciton to set a value based on the $dir variable. This resolved this issue and allows an author to do likewise in the future.

…lKrony/bootstrap into users/adamsiv/rtl-support

DigitalKrony · 2018-09-05T00:26:41Z

@MartijnCuppens - Thanks for taking the time. Can you do a pull and let me know if you see anything else. I'd like to make sure we have this right when it's some to get it merged in. I have a lot of folks on my end REALLY looking forward to this being merged and released.

MartijnCuppens · 2018-09-08T13:35:39Z

@DigitalKrony, I'm glad you're so driven to help Bootstrap with the RTL support but I'm afraid I can only review a limited part of this PR. I'm not familiar with rlt languages myself and I think we'll need reviews from several people who are used to make rtl websites before a PR can be merged.

powerchelle · 2018-11-06T04:51:00Z

@mdo hey there, any idea when this might be integrated?

superware · 2018-11-10T14:53:03Z

@DigitalKrony, I'm glad you're so driven to help Bootstrap with the RTL support but I'm afraid I can only review a limited part of this PR. I'm not familiar with rlt languages myself and I think we'll need reviews from several people who are used to make rtl websites before a PR can be merged.

RTL is easy, in principal think of it as a mirror for LTR content, anything going from left-to-right should go from right-to-left (so right is "preceding" and left is "following"), anything aligned to the left should be aligned to the right, any left-arrow (indicating "previous") should be a right-arrow, etc. "Mirrored" is the key, it's pretty straight forward. See bootstrap.rtlcss.com for example.

BoFFire · 2018-11-24T22:35:01Z

Crazy threads about RTL in Bootstrap :))

superware · 2018-11-25T07:08:36Z

Crazy threads about RTL in Bootstrap :))

There is no technical reason for Bootstrap's lack of RTL support, with around 540 million Arabic/Persian/Hebrew speakers worldwide - no question regarding the necessity.

I was just pointing out the obvious, that RTL is a visual mirror of LTR (alignment, floating, margin, padding wise etc). This has nothing to do with the languages themselves and can be fully supported and tested with English.

ghost · 2018-11-26T14:59:56Z

Zurb Foundation for Sites 6 has a very long experience in RTL support. And they enable RTL by one $global-text-direction variable and then let SASS decide the right direction for $right and $left based on $global-text-direction variable .

$global-text-direction: ltr !default;
$global-left: if($global-text-direction == rtl, right, left);
$global-right: if($global-text-direction == rtl, left, right);

Bootstrap team is waiting for the perfect solution to support RTL, which will not come in the near future or maybe ever?.

powerchelle · 2019-01-17T19:41:16Z

@mdo hi there, we chatted a few months ago about seeing if this could be included in the next minor release. Could you please provide an update?

powerchelle · 2019-04-08T15:02:22Z

@mdo will this be merged in the future, or should we consider this dead?

mdo · 2020-06-16T03:29:38Z

We're looking at tackling RTL in v5! See #30980 for details. Sorry we didn't get to do this for v4, maybe we'll still be able to bridge these major versions once we tackle this in v5. Please weigh in on the new PR and referenced issues with any feedback moving forward! <3

Adam Sivins and others added 5 commits July 31, 2018 16:37

initial pass for converting right and left to string.

cf64b11

Tooltip and Dropdown RTL JS support

ee08c2b

set the docs and SCSS to the LTR default.

7fbfede

spelling

517b90b

Merge branch 'v4-dev' into users/adamsiv/rtl-support

e1d2842

Adam Sivins added 5 commits August 6, 2018 14:26

lint error clean up;

ef6bb1a

Merge branch 'users/adamsiv/rtl-support' of https://github.com/Digita…

96a7475

…lKrony/bootstrap into users/adamsiv/rtl-support

lint and build

1407d20

add Util test for page direction.

baed691

missed test description.

6b3a7ae

dist reset

a82bd3b

Adam Sivins added 2 commits August 7, 2018 09:48

dist reset

ae59fe7

Merge branch 'users/adamsiv/rtl-support' of https://github.com/Digita…

ba21107

…lKrony/bootstrap into users/adamsiv/rtl-support

Merge branch 'v4-dev' into users/adamsiv/rtl-support

c7cbf83

XhmikosR reviewed Aug 7, 2018

View reviewed changes

XhmikosR and others added 3 commits August 7, 2018 10:42

Merge branch 'v4-dev' into users/adamsiv/rtl-support

0b297cd

Merge branch 'users/adamsiv/rtl-support' of https://github.com/Digita…

eb1fe54

…lKrony/bootstrap into users/adamsiv/rtl-support

util getPageDirection updated to be better.

dd94d11

andresgalante added feature css v4 labels Aug 8, 2018

Merge branch 'v4-dev' into users/adamsiv/rtl-support

25f6430

fomr $left to $start and $right to $end

f02e7cf

Adam Sivins and others added 2 commits August 28, 2018 17:29

Merge branch 'v4-dev' into users/adamsiv/rtl-support

517beeb

# Conflicts: # scss/_carousel.scss # scss/mixins/_grid-framework.scss # scss/mixins/_grid.scss # site/_layouts/redirect.html

Merge branch 'v4-dev' into users/adamsiv/rtl-support

c424555

Adam Sivins added 2 commits September 4, 2018 13:35

double check componenets after mergers with master.

37d2598

correct drop left/right menus

3b1b849

DigitalKrony and others added 3 commits September 4, 2018 15:35

Merge branch 'v4-dev' into users/adamsiv/rtl-support

05e7571

Merge branch 'v4-dev' into users/adamsiv/rtl-support

9bf276a

Merge branch 'users/adamsiv/rtl-support' of https://github.com/Digita…

499ab6f

…lKrony/bootstrap into users/adamsiv/rtl-support

DigitalKrony added 2 commits September 10, 2018 14:15

Merge branch 'v4-dev' into users/adamsiv/rtl-support

2caadcd

Merge branch 'v4-dev' into users/adamsiv/rtl-support

1c8051b

powerchelle mentioned this pull request Sep 18, 2018

Bootstrap RTL mdo/ama#285

Closed

XhmikosR mentioned this pull request Nov 4, 2018

Adding RTL support. #26722

Closed

mdo changed the base branch from v4-dev to master March 13, 2019 17:43

mdo closed this Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

users/adamsiv/rtl-support #27027

users/adamsiv/rtl-support #27027

DigitalKrony commented Aug 6, 2018

wolfy1339 commented Aug 6, 2018

wolfy1339 commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR Aug 7, 2018

DigitalKrony Aug 7, 2018

XhmikosR Aug 7, 2018

XhmikosR Aug 7, 2018

DigitalKrony Aug 7, 2018

andresgalante commented Aug 8, 2018

DigitalKrony commented Aug 8, 2018

andresgalante commented Aug 9, 2018

DigitalKrony commented Aug 29, 2018

XhmikosR commented Aug 29, 2018

MartijnCuppens commented Sep 3, 2018

DigitalKrony commented Sep 4, 2018 •

edited

DigitalKrony commented Sep 4, 2018

DigitalKrony commented Sep 5, 2018 •

edited

MartijnCuppens commented Sep 8, 2018

powerchelle commented Nov 6, 2018

superware commented Nov 10, 2018

BoFFire commented Nov 24, 2018

superware commented Nov 25, 2018

ghost commented Nov 26, 2018

powerchelle commented Jan 17, 2019

powerchelle commented Apr 8, 2019

mdo commented Jun 16, 2020

users/adamsiv/rtl-support #27027

users/adamsiv/rtl-support #27027

Conversation

DigitalKrony commented Aug 6, 2018

wolfy1339 commented Aug 6, 2018

wolfy1339 commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR commented Aug 7, 2018

XhmikosR Aug 7, 2018

Choose a reason for hiding this comment

DigitalKrony Aug 7, 2018

Choose a reason for hiding this comment

XhmikosR Aug 7, 2018

Choose a reason for hiding this comment

XhmikosR Aug 7, 2018

Choose a reason for hiding this comment

DigitalKrony Aug 7, 2018

Choose a reason for hiding this comment

andresgalante commented Aug 8, 2018

DigitalKrony commented Aug 8, 2018

andresgalante commented Aug 9, 2018

DigitalKrony commented Aug 29, 2018

XhmikosR commented Aug 29, 2018

MartijnCuppens commented Sep 3, 2018

DigitalKrony commented Sep 4, 2018 • edited

DigitalKrony commented Sep 4, 2018

DigitalKrony commented Sep 5, 2018 • edited

MartijnCuppens commented Sep 8, 2018

powerchelle commented Nov 6, 2018

superware commented Nov 10, 2018

BoFFire commented Nov 24, 2018

superware commented Nov 25, 2018

ghost commented Nov 26, 2018

powerchelle commented Jan 17, 2019

powerchelle commented Apr 8, 2019

mdo commented Jun 16, 2020

DigitalKrony commented Sep 4, 2018 •

edited

DigitalKrony commented Sep 5, 2018 •

edited