floating label rtl fix #11324

sijav · 2017-04-22T20:49:29Z

Add RTL support to Floating labels

Short description of what this resolves:

RTL support for floating labels

Changes proposed in this pull request:

added a scss code which set transform-origin to right for RTL direction on html.

Ionic Version: 2.x / 3.x

add rtl fix to floating label

AmitMY · 2017-04-22T20:59:08Z

Thank you for creating a pull request for RTL, its great to see more people helping on that.
As this is your first contribution, please read the "creating a pull request" guide, and commit message style under it.

This is a great change, but it is missing all-platforms support.
There is also .label-ios[floating] and .label-wp[floating] that need to be addressed.

AmitMY · 2017-04-22T21:00:05Z

src/components/label/label.md.scss

@@ -48,6 +48,12 @@ $label-md-margin:                     $item-md-padding-top ($item-md-padding-rig
  transition: transform 150ms ease-in-out;
 }

+html[dir=rtl] {


Preferably, use [dir="rtl"] instead, to give the option to use rtl on any html component

AmitMY · 2017-04-22T21:01:17Z

src/components/label/label.md.scss

@@ -48,6 +48,12 @@ $label-md-margin:                     $item-md-padding-top ($item-md-padding-rig
  transition: transform 150ms ease-in-out;
 }

+html[dir=rtl] {
+  .label-md[floating] {


When it is a single block, preferably, use [dir="rtl"] .label-md[floating] instead of nesting. (see segment)

problems with transform-origin with both windows and ios platform for rtl and also wrong translate3d for rtl for windows platform

AmitMY · 2017-04-22T22:02:57Z

src/components/label/label.wp.scss

@@ -56,6 +61,11 @@ $label-wp-text-color-focused:         color($colors-wp, primary) !default;
  transform: translate3d(0, 0, 0) scale(.8);
 }

+html[dir="rtl"] .input-has-focus .label-wp[floating],


This rtl behavior is exactly the same as ltr behavior, so no need to write it again (entire block)

actually it's needed as the [dir="rtl"] .label-wp[floating] transform: translate3d will replace the .input-has-focus .label-wp[floating] transform: translate3d on rtl

AmitMY · 2017-04-22T22:03:15Z

src/components/label/label.wp.scss

@@ -40,6 +40,11 @@ $label-wp-text-color-focused:         color($colors-wp, primary) !default;
  transform-origin: left top;
 }

+html[dir="rtl"] .label-wp[floating] {
+  transform: translate3d(-8px, 34px, 0);


This rtl behavior is exactly the same as ltr behavior, so no need to write it again (this specific line)

The ltr translate 8px to the right, this one translate it to the left (-8px instead of 8px), am I missing something here?

No, I was missing it. Thanks

AmitMY · 2017-04-22T22:03:38Z

src/components/label/label.md.scss

@@ -48,6 +48,10 @@ $label-md-margin:                     $item-md-padding-top ($item-md-padding-rig
  transition: transform 150ms ease-in-out;
 }

+html[dir="rtl"] .label-md[floating] {
+    transform-origin: right top;


Indentation is 4 spaces instead of 2

remove reference of "html", just plain '[dir="rtl"] indent 2 space instead of 4

AmitMY · 2017-04-22T22:11:49Z

src/components/label/label.wp.scss

@@ -61,8 +61,8 @@ html[dir="rtl"] .label-wp[floating] {
  transform: translate3d(0, 0, 0) scale(.8);
 }

-html[dir="rtl"] .input-has-focus .label-wp[floating],
-html[dir="rtl"] .input-has-value .label-wp[floating] {
+[dir="rtl"] .input-has-focus .label-wp[floating],


This one still looks the same to me, both for ltr and rtl

because I've added a transform: translate3d with more specific reference then a class (an attribute) it will replace the original LTR (.input-has-focus or .input-has-value), with default one so the label won't float on RTL direction, therefore I've added the same thing with attribute specific (dir="rtl") ... it's hackish, I know, but couldn't find a better suitable easy way.

I'm not sure what you mean...
I am referring to:

.input-has-focus .label-wp[floating], .input-has-value .label-wp[floating] { transform: translate3d(0, 0, 0) scale(.8); } [dir="rtl"] .input-has-focus .label-wp[floating], [dir="rtl"] .input-has-value .label-wp[floating] { transform: translate3d(0, 0, 0) scale(.8); }

Which as I see it, is equivalent to:

.input-has-focus .label-wp[floating], .input-has-value .label-wp[floating] { transform: translate3d(0, 0, 0) scale(.8); }

Because you override the ltr with the same behavior

Easy,
unfortunately the browser will choose this rule

[dir="rtl"] .label-wp[floating] { transform: translate3d(-8px, 34px, 0); }

over this one

.input-has-focus .label-wp[floating], .input-has-value .label-wp[floating] { transform: translate3d(0, 0, 0) scale(.8); }

even if the label has a parent element with input-has-focus class because of the attribute specification from previous rule which has a higher priority than a class. so I've added exactly the same thing over again with attribute specification, so the browser choose the right one on focus or when the input has value in rtl mode...
This can be easily seen in a test, try to remove those three line code and test a wp platform you'll see the floating label won't float and won't go up on focus.

Hmm.. ok.
I might nor know enough about CSS priorities, but from a small test, it seems like it goes like I said it would: https://codepen.io/anon/pen/BRzxza

As far as I understand, that is because of the order of selectors:
.input-has-focus .label-wp[floating] is after [dir="rtl"] .label-wp[floating] and thus takes president.

Let me know if you still think otherwise (as again, I am not completely sure of it)

Yes, you're right, sorry, my bad, my way of testing this was wrong!

remove unnecessary duplicate rule

AmitMY · 2017-04-22T23:14:50Z

@manucorporat @brandyscarney This looks done. Can you merge? (and add rtl label)

sijav · 2017-04-23T10:02:07Z

I'm creating another pr which contains this too.
So I'm closing this.

AmitMY · 2017-04-23T10:09:32Z

In theory, PRs should be "Small, independent, complete contributions" such that it is preferable to have 2 different PRs rather than 1 PR with 2 features.
If your new PR fixes more labels, than it should be 1 PR, otherwise 2.

If you are interested in learning more -

Read: http://dl.acm.org/citation.cfm?id=2594458
Or view it presented: https://www.youtube.com/watch?v=48TjLDUf-B4

sijav · 2017-04-23T10:24:13Z

@AmitMY So I should open this and make some new other PR for my other changes? sorry I'm new to this.

AmitMY · 2017-04-23T10:25:56Z

@sijav No problem :)
Yes, imo, and the theory suggest, that is better if you open it and create a new PR for any other feature.

I really recommend that video, it explains a lot about open source PRs and the review process

sijav · 2017-04-23T10:28:10Z

Yes, Thanks a lot,

brandyscarney · 2017-04-24T15:51:45Z

This looks great! Thank you @sijav for the PR, and @AmitMY for doing a great job reviewing and providing feedback. 🙂 🎉

floating label rtl fix

fc61d2e

add rtl fix to floating label

AmitMY reviewed Apr 22, 2017

View reviewed changes

sijav added 2 commits April 23, 2017 01:57

single block, not nesting

dbbea5a

fix(floating-label): wp and ios issue for rtl

650f7ae

problems with transform-origin with both windows and ios platform for rtl and also wrong translate3d for rtl for windows platform

AmitMY reviewed Apr 22, 2017

View reviewed changes

style(floating-label): remove html, 2 space indent

c0c4c5c

remove reference of "html", just plain '[dir="rtl"] indent 2 space instead of 4

AmitMY reviewed Apr 22, 2017

View reviewed changes

refactor(floating-label): remove duplicate rule

e1295ef

remove unnecessary duplicate rule

sijav closed this Apr 23, 2017

sijav reopened this Apr 23, 2017

manucorporat added the rtl label Apr 24, 2017

brandyscarney merged commit 0ec71cd into ionic-team:master Apr 24, 2017

sijav deleted the floating-rtl-fix branch April 25, 2017 04:30

AmitMY mentioned this pull request May 12, 2017

ion-label floating direction rtl , the label come almost in the middle not at the right. #11627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

floating label rtl fix #11324

floating label rtl fix #11324

sijav commented Apr 22, 2017

AmitMY commented Apr 22, 2017

AmitMY Apr 22, 2017

AmitMY Apr 22, 2017

AmitMY Apr 22, 2017 •

edited

Loading

sijav Apr 22, 2017 •

edited

Loading

AmitMY Apr 22, 2017

sijav Apr 22, 2017

AmitMY Apr 22, 2017

AmitMY Apr 22, 2017

AmitMY Apr 22, 2017

sijav Apr 22, 2017

AmitMY Apr 22, 2017

sijav Apr 22, 2017 •

edited

Loading

AmitMY Apr 22, 2017

sijav Apr 22, 2017

AmitMY commented Apr 22, 2017 •

edited

Loading

sijav commented Apr 23, 2017

AmitMY commented Apr 23, 2017

sijav commented Apr 23, 2017

AmitMY commented Apr 23, 2017

sijav commented Apr 23, 2017

brandyscarney commented Apr 24, 2017

floating label rtl fix #11324

floating label rtl fix #11324

Conversation

sijav commented Apr 22, 2017

Short description of what this resolves:

Changes proposed in this pull request:

AmitMY commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitMY Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

sijav Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijav Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitMY commented Apr 22, 2017 • edited Loading

sijav commented Apr 23, 2017

AmitMY commented Apr 23, 2017

sijav commented Apr 23, 2017

AmitMY commented Apr 23, 2017

sijav commented Apr 23, 2017

brandyscarney commented Apr 24, 2017

AmitMY Apr 22, 2017 •

edited

Loading

sijav Apr 22, 2017 •

edited

Loading

sijav Apr 22, 2017 •

edited

Loading

AmitMY commented Apr 22, 2017 •

edited

Loading