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 #12071

Closed
wants to merge 28 commits into from

Conversation

sijav
Copy link
Contributor

@sijav sijav commented Jun 16, 2017

Short description of what this resolves:

This will fix bubbles and circles spinner on RTL

Changes proposed in this pull request:

  • as spinners are only clockwise we should not use position mixin and instead use multi-dir mixin

Ionic Version: 3.x

Fixes: #11211

brandyscarney and others added 28 commits June 12, 2017 13:51
When `Tabs` are nested within each other, the highlight can get
misaligned. This prevents that by ensuring the affected
`.tab-highlight` is a direct child of the targeted `Tabs`.
@sijav
Copy link
Contributor Author

sijav commented Sep 29, 2017

@AmitMY @brandyscarney
any update for this?
we still have this bug for 3.7.1 on RTL

@AmitMY
Copy link
Contributor

AmitMY commented Oct 1, 2017

@kensodemann It shouldn't be hard to merge this both in master and in core. Can you please see if this can be done for the next version?

@kensodemann
Copy link
Member

LGTM, I am fine merging it if @brandyscarney is.

Question: would this be more maintainable?

@include multi-dir() {
  @include position(0, null, null, 0);
}

I wasn't completely sure if that would work once I started following the chain of mixins down.

@sijav
Copy link
Contributor Author

sijav commented Oct 1, 2017

@kensodemann but putting position mixin inside multi-dir mixin is not make any sense. let me explain the problem here.
The whole problem with bubbles and circles is that on spinner.ts we use left style (regardless of site's direction RTL or LTR) for placing the dots SVGs on the correct position, now when we use position mixin it would result in placing right: 0; in the css instead of left style on RTL so the left style on SVG element won't replace the inheritance hence they always position at right: 0; on RTL regardless of different left style on SVG element itself.

Bottom line is we have two options:

  1. modify spinner.ts file and change that it checks for direction (RTL and LTR) before putting left style on SVG elements and use right style in case of RTL.
  2. modify spinner.scss file and hard-codedly put left: 0; regardless of site's direction the same as .ts.
    As sass modifications is always preferred, I think changing sass and putting hard-coded left value is not a total bad idea.

@AmitMY
Copy link
Contributor

AmitMY commented Oct 1, 2017

@kensodemann It was a bit before your time, so just to let you know, I am the one who wrote the SCSS Mixins for directional support, signed off by brandy.
This looks good to me.

I made this entire process semi automatic, so issues like this happen, and they are my bad.
This is the best fix for this situation

@kensodemann
Copy link
Member

@sijav - thank you for the explanation, that was helpful

@Ionitron
Copy link
Collaborator

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!

@Ionitron Ionitron closed this Mar 12, 2018
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.