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(button): rtl fix for md ripple effect #11842

Merged
merged 9 commits into from
Jun 7, 2017

Conversation

sijav
Copy link
Contributor

@sijav sijav commented May 29, 2017

Short description of what this resolves:

This will fix ripple effect on material design which were showing displaced of touch click.

Changes proposed in this pull request:

  • remove the include position and instead use absolute values.

Ionic Version: 3.x

Fixes: #11211

@include position(0, null, null, 0);
// scss-lint:disable PropertySpelling
left: 0;
top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

First, know that this won't pass linting, as the properties are above an @include

Because of the new changes, this needs to be inside a @include multi-dir() { which goes at the end of the selector. When that is done, it can be merged. (Didn't merge before as I knew there will be changes with multi-dir)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I get it now thanks. Will do

@sijav
Copy link
Contributor Author

sijav commented Jun 7, 2017

@AmitMY is it alright now?


@include multi-dir() {
top: 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this empty line + move //scss-lint:disable... to the first row of the mixin? something like

@include multi-dir() {
  // scss-lint:disable PropertySpelling
  top: 0;
  left: 0;
}

(because we don't won't to ignore it for the entire block)

Copy link
Contributor Author

@sijav sijav Jun 7, 2017

Choose a reason for hiding this comment

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

@AmitMY sure but css lint gave me error because I hadn't put empty line in between ... I think it was because I had order problems? I'll commit the new changes asap

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sure, linter rules over me ;)
So just that disable comment if you can

@sijav
Copy link
Contributor Author

sijav commented Jun 7, 2017

@AmitMY With this commit I get this PropertySortOrder: Property 'left' should have an empty line separating it from the previous group of properties ending with 'top'

@AmitMY
Copy link
Contributor

AmitMY commented Jun 7, 2017

@sijav

Oh sure, linter rules over me ;)

so final thing here, add an empty line there, and this seems good. Sorry for the messiness

@brandyscarney brandyscarney merged commit bb966e5 into ionic-team:master Jun 7, 2017
@brandyscarney
Copy link
Member

Looks great, thank you!

@sijav sijav deleted the rtl-fix-button-md-effect branch June 8, 2017 16:00
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