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): better RTL support #11121

Closed
wants to merge 11 commits into from
Closed

feat(RTL): better RTL support #11121

wants to merge 11 commits into from

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Apr 7, 2017

Short description of what this resolves:

  • Much better RTL support

Changes proposed in this pull request:

  • Add text-start and text-end. Work exactly the same on ltr, but flips directions on rtl.
    This also avoids confusion, when doing text-left on rtl it means left, but text-start means right
  • Add pull-right and pull-left (bootstrap style naming) for float, and flipped on RTL
  • Flip every icon except for logos and specific icons that are the same in RTL. (I am from Israel, I made the list)
  • Flip tabs border-radius - fixes
    image
  • Flip icon-left and icon-right padding

Ionic Version: 3.x

Fixes:
Partially: #11115
Fully: #10685

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 9, 2017

Can I get any comment on this? Am I doing something wrong? is this to be merged sometime, should I change anything?

Should I split this into 5 separate PRs?

Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some feedback within the PR. Please let me know your thoughts! Thanks!

// Float
// --------------------------------------------------

[pull-left],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do pull-left, pull-right, pull-start, and pull-end here and make them work like the text-* attributes do? Or is there a reason not to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, moved to a separate PR: #11214

padding-right: initial;
}

[dir="rtl"] [icon-right] ion-icon {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same question as above with icon-*.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's gonna be a problem.
icon-left is for CSS, so when you do:

<button ion-button icon-left>
    Button
    <ion-icon name="chatbubbles"></ion-icon>
</button>

It doesn't move the icon to the left, just adds some wrong padding.

@@ -8,6 +8,7 @@ $ionicons-font-path: $font-path !default;

@import "ionicons-icons";
@import "ionicons-variables";
@import "~hail2u-scss-functions/string/_str-replace";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this for icons. I wonder if there is a better way to decide if the icon should or shouldn't be flipped on rtl. We're thinking maybe we could add some metadata to the icon itself and look at it that way? We need to discuss this more but maybe this could be split out from this PR for further discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. ok
How about for starters, do:
ion-icon[area-label^="arrow"] { @extends [flip] } ?
To be honest, most of the icons don't matter, its just the arrows that are going the other way..

@brandyscarney
Copy link
Member

@AmitMY Yeah could you split this into separate PRs. It's easier to merge them and review them that way. 😄

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 13, 2017

@brandyscarney Thanks.
All 5 PRs created, and are easily mergable (perhaps except for icon-x which probably requires further discussion)

@AmitMY AmitMY closed this Apr 13, 2017
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.

2 participants