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(spinner): RTL fix for Spinner #12069

Closed
wants to merge 3 commits into from
Closed

Conversation

sijav
Copy link
Contributor

@sijav sijav commented Jun 16, 2017

Short description of what this resolves:

This will make sure that the spinners will go anti-clockwise in RTL mode
This will also fix bubbles and circles spinner on rtl

Changes proposed in this pull request:

  • import platform and add it to spinner class
  • put SPINNERS const variable in spinner class's load function and add RTL support to it's functions.
  • add spinner-rotate-rtl keyframes animation and assign rtl mixins to related selector

Ionic Version: 3.x

Fixes: #11211

@AmitMY
Copy link
Contributor

AmitMY commented Jun 16, 2017

Wow I didn't even notice, on a phone are they actually reversed?

This seems overly complicated, perhaps it can be simplified to just on RTL have "transform: scaleX(-1)"?

Not sure, and I can't test it today, but I think it would work, like range & progress bar.

Or perhaps, we can just make the rotate into the transform mixin. On ltr X is (360-X) on rtl. That seems like a general solution for all of the rotation animations that exist.

I will properly review this tomorrow night.

@sijav
Copy link
Contributor Author

sijav commented Jun 16, 2017

@AmitMY I just checked android's Play Store on RTL it was not anti clockwise so I guess this thing is not necessary.
Though we still have problems with bubbles and circles spinner when on RTL I'll create another PR asap.
Thanks.

@sijav sijav closed this Jun 16, 2017
@sijav sijav deleted the rtl-fix-spinner branch June 16, 2017 17:29
@AmitMY
Copy link
Contributor

AmitMY commented Jun 16, 2017

Ok good to know
If you are looking for something to fix, can you take a look at animated search bar on iOS RTL? It does weird things

@sijav
Copy link
Contributor Author

sijav commented Jun 16, 2017

@AmitMY we are using include padding and include margin-horizontal in searchbar I thought you know it yourself they are not working correctly, not even background position mixin or transform thing. It's nothing that I can understand or have enough information about to handle it. sorry. if it were simple scss, fixing it would take less than an hour :D

@AmitMY
Copy link
Contributor

AmitMY commented Jun 16, 2017

I was not aware padding and margin horizontal don't work correctly. Can you explain a bit more?

Current state is that search bar works great, other than animated search bar which breaks the view

@sijav
Copy link
Contributor Author

sijav commented Jun 17, 2017

@AmitMY for instance we have =>

.searchbar-ios .searchbar-ios-cancel {
  @include padding(0, 0, 0, 8px);
  @include margin-horizontal(0, null);
  ...
}

what it will be compiled is =>

.searchbar-ios .searchbar-ios-cancel {
    padding: 0 0 0 8px;
    margin-left: 0;

see? no specified [rtl] or [ltr] attribute added in it
EDIT: I made it once with following scss
EDIT 2: the whole searchbar does not seem to have rtl or ltr specified compiled css

@AmitMY
Copy link
Contributor

AmitMY commented Jun 17, 2017

Make sure $app-direction is set to "multi".
V3.4.1/3.4.2 changed the default to ltr, because the [dir=..] breaks some specificity rules for users

@AmitMY
Copy link
Contributor

AmitMY commented Jun 17, 2017

Weird, I'll check the bundle with no multi.
iOS searchbar - great. Last time I checked it really sucked. I'll verify. Thanks!

@sijav
Copy link
Contributor Author

sijav commented Jun 17, 2017

@AmitMY I've removed my comment, I'm checking it again something is wierd I'll let you know

@sijav
Copy link
Contributor Author

sijav commented Jun 17, 2017

@AmitMY nothing is wrong, it was chrome bug with rtl where it place the scroll bar at left instead of right in rtl mode and when we switch to phone view chrome still keeps the scroll bar intact which is not alright. nothing that we can do though

@sijav
Copy link
Contributor Author

sijav commented Jun 17, 2017

@AmitMY please check with no multi no ltr and rtl no nothing we will face many [dir=...] in the compiled main.css but not all which might breaks the view.

@AmitMY
Copy link
Contributor

AmitMY commented Jun 17, 2017

About the segment, you are correct. it is a non-issue then.
About direction - if I keep it default (ltr) there are 2 manual rtl selectors added, for border width on segment. these are fixed in a PR.

If I set it to nothing (can't nothing, so 0 for example), it has no directional behaviour at all. no ltr, nor rtl.

I'm just now adding a check for "is the current direction loaded"

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