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.

Sign up for GitHub

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

Jump to bottom

Final RTL discussion #28797

Closed
zjbarg opened this issue May 21, 2019 · 5 comments
Closed

Final RTL discussion #28797

zjbarg opened this issue May 21, 2019 · 5 comments
Labels
css rtl

Comments

@zjbarg
Copy link

zjbarg commented May 21, 2019 •
edited
Loading

The issue:

Bootstrap doesn't currently have rtl support. The solution should come from the rtl-interested community, but we need a plan. Maintaining a fork with rtl fixes is, in my opinion, wasted effort; there must be a better solution.

Approach for solving this issue:

Rules

  1. Same source ( No abc.rtl.scss).
  2. Should not increase the dist size at all for users who do not need rtl support.

Solution

  1. New Variable $rtl-support
Value Meaning
0 Original bootstrap (default)
1 Everything filpped
2 LTR and RTL**

** When using 2 user will have to set the dir="rtl" or dir="ltr". i.e <html dir="ltr"> or <html dir="rtl">

  1. Mixins everywhere
    Instead of
.alert-dismissible {
  padding-right: $close-font-size + $alert-padding-x * 2;

  // Adjust close link position
  .close {
    position: absolute;
    top: 0;
    right: right;
    padding: $alert-padding-y $alert-padding-x;
    color: inherit;
  }
}

lets have

.alert-dismissible {
  @include padding-right($close-font-size + $alert-padding-x * 2);

  // Adjust close link position
  .close {
    position: absolute;
    top: 0;
    @include right(0);
    padding: $alert-padding-y $alert-padding-x;
    color: inherit;
  }
}

and the mixins would look like:

@mixin padding-right($value) {
  @if ($rtl-support == 0) {
    padding-right: $value;
  }
  @else if ($rtl-support == 1) {
    padding-left: $value;
  }
  @else if ($rtl-support == 2) {
    [dir="ltr"] & {
      padding-right: $value;
    }
    [dir="rtl"] & {
      padding-left: $value;
    }
  }
}

@mixin right($value) {
  @if ($rtl-support == 0) {
    right: $value;
  }
  @else if ($rtl-support == 1) {
    left: $value;
  }
  @else if ($rtl-support == 2) {
    [dir="ltr"] & {
      right: $value;
    }
    [dir="rtl"] & {
      left: $value;
    }
  }
}

Also, option 2 will include ms-x, ps-x, float-start, and -e, -end classes.

Obviously there will be alot of mixins.
Moreover some of the mixins will change, i.e. border-left-radius => border-start-radius and will contain the same login as shown above.

I didn't want to make a pull request of this so here is an example where I have done the above for [alert, breadcrumb, btn-group, card]

If the maintainers have another approach to proving RTL support, kindly share.

Some of the related issues

#28238 #24807 #27123 #27122 #26879 #26818 #19545 #26299 #25422 #24662 #23703 #24332 #23117 #22708 #22137 #21619 #21180 #20293 #19555 #20075 #19787 #19703 #19668 #19643 #19545 #19379 #18184 #17595 #16455 #16419 #15717 #15700 #15509 #15479 #15433 #14717 #14510 #13564

@mdo @cvrebert
@tmorehouse tmorehouse mentioned this issue Jun 11, 2019
Closed
@tmorehouse
Copy link
Contributor

Another option that might work is to run Bootstrap's CSS through postcss-rtl, which would generate a new CSS file with both LTR and RTL support.

I haven't tried it myself, but it might be an option.

@ronjb04
Copy link

ronjb04 commented Oct 24, 2019

Another option that might work is to run Bootstrap's CSS through postcss-rtl, which would generate a new CSS file with both LTR and RTL support.

I haven't tried it myself, but it might be an option.

i have check this option but it seems to make your css a bit bulky

@ffoodd
Copy link
Member

ffoodd commented Oct 24, 2019

FWIW, we're using rtlcss in our own fork Boosted to output another dist file.

However it still requires a few overrides, which we basically push to the RTL file.

Our script looks like this: rtlcss dist/css/boosted.css dist/css/boosted-rtl.css && shx cat dist/css/o-rtl.css >> dist/css/boosted-rtl.css. This is probably not the best way, but at least it works and we can provide some help with it.

Also, if the targetted version is v5 and if dropping support for IE11 is considered, using logical properties and values everywhere possible could help a lot with RTL, without doing anything…

@nijel nijel mentioned this issue Nov 1, 2019
Open
@ffoodd
Copy link
Member

ffoodd commented Mar 25, 2020

Simple update to mention that v5 dropped IE11 support in #30377 — so logical properties might be a game changer alongside custom properties.

@ysds ysds mentioned this issue Apr 1, 2020
Closed
@ffoodd ffoodd mentioned this issue May 28, 2020
Closed
@zjbarg
Copy link
Author

zjbarg commented May 30, 2020

Discussion of this approach and other approaches to tackle RTL is now in #30918

@zjbarg zjbarg closed this as completed May 30, 2020
@ffoodd ffoodd mentioned this issue Jun 8, 2020
Merged
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
css rtl
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tmorehouse @ffoodd @MartijnCuppens @zjbarg @ronjb04