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

feat(rtl): add rtl margin, padding, position and border-radius #11342

Merged
merged 58 commits into from
May 12, 2017

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Apr 24, 2017

RTL support

The goal is to have full RTL support for all of ionic.
If you don't need RTL support, you can set $include-rtl: false, and your css bundle will be smaller.
This means that every property that controls horizontal sides (left, right) must go away.

Directional mixins

To remove all directional properties, we built mixins to handle them in LTR as default, and for RTL automaically.
The mixins should have the same structure as the full property:
prop($top, $start: $top, $bottom: $top, $end: $start)

Mixins like:

Custom RTL support

When you want to add custom RTL support (sometimes it is needed, you should do:

element {
    @include rtl() {
        // Custom css goes here
    }
}

Future properties

Directional naming

We kept the naming left and right where possible, to actually mean left and right, both on ltr and on rtl, but added new names: start and end where start means left on ltr, right on rtl, and end means right on ltr, left on rtl.

New names
  • item-start & item-end
  • icon-start & icon-end
  • side="start" / side="end" for ion-menu (defaults to start)
  • padding-start & padding-end
  • margin-start & margin-end
  • start and end for FABs
  • flip-rtl and unflip-rtl for ion-icon (arrows are flipped by default)
Deprecated names
  • item-left & item-right
  • icon-left & icon-right

Disabled properties

  • padding

  • padding-left

  • padding-right

  • margin

  • margin-left

  • margin-right

  • right

  • left

  • border-radius

  • border-top-left-radius

  • border-top-right-radius

  • border-bottom-right-radius

  • border-bottom-left-radius

  • text-align

Deprecated variables

We decided that to get the best support we need to split variables like $item-margin: 0 1px 2px 3px to:

  • $item-margin-top: 0;
  • $item-margin-end: 1px;
  • $item-margin-bottom: 2px;
  • $item-margin-start: 3px;

And to change many variables with left or right with them to have start or end instead.
So we deprecated many variables.

In ionic v3.x, we offer compatibility for deprecated variables, so if you used them, nothing will change in your app. However, in v4 they are planned to be removed

Deprecated test:

// deprecated
$var:           5px !default;
/// @prop - Margin top
$var-top:       1px !default;
/// @prop - Margin end
$var-end:       2px !default;
/// @prop - Margin bottom
$var-bottom:    3px !default;
/// @prop - Margin start
$var-start:     4px !default;

.a {
  @include deprecated-variable(margin, $var) {
    @include margin($var-top, $var-end, $var-bottom, $var-start);
  }
}

$var: null;
.b {
  @include deprecated-variable(margin, $var) {
    @include margin($var-top, $var-end, $var-bottom, $var-start);
  }
}

Results in:

.a {
  margin: 5px;
}

.b {
  margin-left: 4px;
  margin-right: 2px;
  margin-top: 1px;
  margin-bottom: 3px;
}
[dir="rtl"] .b {
  margin-right: 4px;
  margin-left: 2px;
}

Which is correct

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 24, 2017

@manucorporat Holy cow. Can I use those? Is the current Sass build adding all of the correct browsers support?

@manucorporat
Copy link
Contributor

manucorporat commented Apr 24, 2017

@AmitMY I am not sure if we our current setup autoprefix those, but in case it does not, it would not be hard to tweat the configuration (can we add our own auto-prefixes?) maybe a mixin?

I think the support if good enough, but IE and edge, stupid microsoft haha

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 24, 2017

Ok.. Waiting for @brandyscarney to make sure this is the right way, because this looks awesome, but might not be fully supported

add support for old variable names
@brandyscarney
Copy link
Member

Hey @AmitMY, can we add you to our Slack? Do you have an e-mail we could add?

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 24, 2017

I updated the PR to have the padding-block-start and to fix variable names. Let me know what you think.

@brandyscarney Sure, I can try slack.

@brandyscarney
Copy link
Member

@AmitMY Awesome, we'll be sending you an email to add you shortly. 🙂 I edited your email out of the comment.

@AmitMY AmitMY changed the title feat(rtl): fix all start and end feat(rtl): fix all start and end padding Apr 25, 2017
AmitMY added 5 commits May 5, 2017 18:31
# Conflicts:
#	src/components/checkbox/checkbox.ios.scss
#	src/components/checkbox/checkbox.md.scss
#	src/components/checkbox/checkbox.wp.scss
#	src/components/item/item.ios.scss
#	src/components/item/item.md.scss
#	src/components/item/item.wp.scss
#	src/components/radio/radio.ios.scss
#	src/components/radio/radio.md.scss
#	src/components/radio/radio.wp.scss
#	src/components/toggle/toggle.ios.scss
#	src/components/toggle/toggle.md.scss
#	src/components/toggle/toggle.wp.scss
# Conflicts:
#	src/components/checkbox/checkbox.ios.scss
#	src/components/checkbox/checkbox.md.scss
#	src/components/checkbox/checkbox.wp.scss
#	src/components/item/item.ios.scss
#	src/components/item/item.md.scss
#	src/components/item/item.wp.scss
#	src/components/radio/radio.ios.scss
#	src/components/radio/radio.md.scss
#	src/components/radio/radio.wp.scss
#	src/components/toggle/toggle.ios.scss
#	src/components/toggle/toggle.md.scss
#	src/components/toggle/toggle.wp.scss
# Conflicts:
#	src/components/checkbox/checkbox.ios.scss
#	src/components/checkbox/checkbox.md.scss
#	src/components/checkbox/checkbox.wp.scss
#	src/components/item/item.ios.scss
#	src/components/item/item.md.scss
#	src/components/item/item.wp.scss
#	src/components/radio/radio.ios.scss
#	src/components/radio/radio.md.scss
#	src/components/radio/radio.wp.scss
#	src/components/toggle/toggle.ios.scss
#	src/components/toggle/toggle.md.scss
#	src/components/toggle/toggle.wp.scss
sijav added a commit to sijav/ionic that referenced this pull request May 5, 2017
PR - ionic-team#11342 - covers the scss, and is about done
@brandyscarney brandyscarney merged commit a30379b into ionic-team:master May 12, 2017
sijav added a commit to sijav/ionic that referenced this pull request May 13, 2017
sijav added a commit to sijav/ionic that referenced this pull request May 13, 2017
Remove left right padding margin in favor of ionic-team#11342
brandyscarney pushed a commit that referenced this pull request May 17, 2017
* fix(select): RTL fix for searchbar

RTL fix for searchbar component

* fix bad reference

I forgot to refrence _isRTL

* use platform variable instead of method

* space indent instead of tab indent

* Remove scss changes per request

PR - #11342 - covers the scss, and is about done
@alaa-alshamy
Copy link

alaa-alshamy commented May 28, 2017

@AmitMY hello ,, thanks for this awesome pull request but there's some bugs in it

1- the side menu has been broken in RTL mode after this pull request because u reverse it's position but u shouldn't because when side of the menu "right" it really mean it should be on the right and it was working perfect but after this pull request u reverse it's position to be left in RTL mode

2- horizontal margin of the icons inside items removed in ios with RTL because the mixin of horizontal margin making the other side "initial" so it remove the margin in this case

@AmitMY
Copy link
Contributor Author

AmitMY commented May 28, 2017

@alaa-alshamy You are correct, sorry for that, it was too big, it had to fail somewhere :)
About menu - you are correct. This fixes it, and I think I'll separate it just so it will be merged faster.
About margin - this PR adds a lot of RTL only stuff, so in this case, LTR is overriding some of the margins, paddings, etc. This PR #11635 will be merged soon, and it includes a full separations of styles for LTR and RTL (no overrides)

As you know, RTL is very much work in progress. I'm almost done fixing the scss, and @sijav is doing great work on ts changes. When the main changes are merged and a nightly is released, I will ping all of the RTL issue thread users to check it out.

TL;DR: RTL is very much WIP and unstable

@alaa-alshamy
Copy link

yea i understand and really appreciate ur work

nice job bro :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants