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

fix(rtl): flip arrow icons on rtl #11218

Closed
wants to merge 48 commits into from
Closed

fix(rtl): flip arrow icons on rtl #11218

wants to merge 48 commits into from

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Apr 13, 2017

Short description of what this resolves:

bad arrow direction on rtl

Changes proposed in this pull request:

  • flip arrow icons on rtl

Only arrows?

I understand that more icons should be flipped, for example, exit, which is also an arrow, but that should be somehow in the icon's metadata.
I went all out in this edit and flipped all icons that can be flipped without causing harm, so this is just a toned down version

Ionic Version: 3.x

@brandyscarney
Copy link
Member

@AmitMY What if we had an attribute that only flips the icon for rtl if it is added. Something like this:

[dir="rtl"] [flip-rtl] {
  -moz-transform: scaleX(-1);
  -o-transform: scaleX(-1);
  -webkit-transform: scaleX(-1);
  transform: scaleX(-1);
}

Then if you want the icon to be flipped you would add it like:

<ion-icon name="arrow-forward" flip-rtl></ion-icon>

This would make it completely up to the user to flip the icon or not, could be used on icons outside of ionicons, and gets rid of any hardcoded styles for ionicons in case icons are added or removed. Then we would need to add some styling for our icons that are used in components. What do you think of this solution? Would it be viable?

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 13, 2017

@brandyscarney While I think that should be an option (the [flip-rtl] toggles flipping), I also think that some icons, and at least the arrow ones should be auto flipped.

It is a hassle, so most users wont use it.

The arrows mostly use forward and back, and that agrees with the style of start and end. No implicit left and right, so it needs to be flipped on rtl...

Look for the word "arrow" here:
https://ionicframework.com/docs/ionicons/

@AmitMY AmitMY mentioned this pull request Apr 15, 2017
11 tasks
@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 17, 2017

@brandyscarney Well, I added the flip-rtl, and a unflip-rtl, to be extra safe if someone wants to unflip the arrows, but I don't see anyone using the unflip-rtl attribute, as the arrows flipping is basically wanted on most if not all apps

@@ -33,3 +33,20 @@ ion-icon {
text-transform: none;
speak: none;
}

@mixin scale-x($amount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

scss already does it for transform, scale-x is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manucorporat can you please explain a bit more? If I only do transform: scaleX(-1); it does not cover browsers support, so I am not sure what you mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manucorporat I looked in some code, and it seems you never do custom browser support for transform, so I guess I'll do the same. Updated

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 21, 2017

@manucorporat Anything else to fix here?

@AmitMY
Copy link
Contributor Author

AmitMY commented May 3, 2017

@brandyscarney Can you re-review?

AmitMY added 13 commits May 4, 2017 12:56
# Conflicts:
#	src/components/range/range.ios.scss
#	src/components/range/range.md.scss
#	src/components/range/range.wp.scss
#	src/components/slides/slides.scss
# Conflicts:
#	src/components/action-sheet/action-sheet-component.ts
#	src/components/alert/alert.ios.scss
#	src/components/alert/alert.md.scss
#	src/components/alert/alert.wp.scss
#	src/components/tabs/tabs.scss
#	src/components/toolbar/toolbar.ios.scss
#	src/components/toolbar/toolbar.md.scss
#	src/components/toolbar/toolbar.wp.scss
@AmitMY AmitMY closed this May 12, 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.

3 participants