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): icon in rtl should have margin right flipped. #2346

Merged
merged 4 commits into from
Mar 6, 2018

Conversation

aprigogin
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 5, 2018

Codecov Report

Merging #2346 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2346   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files         100      100           
  Lines        4118     4118           
  Branches      527      527           
=======================================
  Hits         4071     4071           
  Misses         47       47

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9000a4...ac4dc6d. Read the comment docs.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

You should also add mdc-rtl to button's dependencies in package.json. (If it's building right now, it's doing so "by accident" via an indirect dependency.)

@acdvorak acdvorak self-assigned this Mar 6, 2018
@@ -16,6 +16,7 @@

@import "@material/elevation/mixins";
@import "@material/ripple/mixins";
@import "@material/rtl/mixins";
Copy link
Contributor

Choose a reason for hiding this comment

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

Add rtl to dependencies in button's package.json

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

@@ -127,6 +128,8 @@
}

@mixin mdc-button__icon_ {
@include mdc-rtl-reflexive-box(margin, right, 8px);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably remove the margin-right: 8px line below (line 136)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, my bad.

@@ -15,6 +15,7 @@
"dependencies": {
"@material/elevation": "^0.28.0",
"@material/ripple": "^0.32.0",
"@material/rtl": "^0.32.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried npm installing this? I think the latest version of mdc-rtl is still 0.30.0 due to no changes since then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not tried it, like i have not tried gazillion other things I could have tried... But I should have checked what rtl version there is. it is 30 not 32.

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

@aprigogin aprigogin merged commit 3c04419 into master Mar 6, 2018
@aprigogin aprigogin deleted the button/rtl branch March 6, 2018 22:29
acdvorak added a commit that referenced this pull request Mar 8, 2018
commit f17e0d3
Author: Will Ernest <[email protected]>
Date:   Thu Mar 8 13:20:48 2018 -0500

    fix(text-field): Clicking label should focus textfield (#2353)

commit 77b15f4
Author: Bonnie Zhou <[email protected]>
Date:   Wed Mar 7 16:29:57 2018 -0800

    refactor(button): Remove compact variant (#2361)

    BREAKING CHANGE: The compact variant of MDC Button is removed.

commit 35a5cfc
Author: Will Ernest <[email protected]>
Date:   Tue Mar 6 19:54:23 2018 -0500

    fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331)

    SVG icons in the toolbar will now use the same color as the font icons.

commit dc52201
Author: Andrew C. Dvorak <[email protected]>
Date:   Tue Mar 6 16:00:12 2018 -0800

    fix(theme): Move @alternate annotations for Closure Stylesheets (#2355)

    `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all).

commit 3c04419
Author: aprigogin <[email protected]>
Date:   Tue Mar 6 14:29:12 2018 -0800

    fix(button): icon in rtl should have margin right flipped. (#2346)

commit b9000a4
Author: Cory Reed <[email protected]>
Date:   Tue Mar 6 07:37:40 2018 -0800

    fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348)

commit ab85736
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 19:00:11 2018 -0500

    fix: Use `var` instead of `const` in menu demo (#2345)

    Safari 9.x and IE 10 do not support `const`

commit dc3d69f
Author: aprigogin <[email protected]>
Date:   Mon Mar 5 15:22:31 2018 -0800

    fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344)

    * fix(rtl): Adding noflip annotations to fix downstream rtl issues

    * fix(rtl): remove changes to button, will be in separate PR

    * fix(rtl): remove changes to button, will be in separate PR - second attempt.

    * fix(rtl): removed extra whitespace

commit eb4138e
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:46 2018 -0800

    docs: Update CHANGELOG.md

commit f478610
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:30 2018 -0800

    chore: Publish

commit 78408bb
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 16:13:02 2018 -0500

    fix: Use `var` instead of `const` in demos/ready.js (#2343)

    Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5.

commit 49a9449
Author: Will Ernest <[email protected]>
Date:   Mon Mar 5 12:21:54 2018 -0500

    chore(select): Fix JS example in Readme (#2332)

commit bc17291
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 13:21:07 2018 -0500

    feat(top app bar): Add short top app bar always collapsed feature (#2327)

commit 0ba7d10
Author: Matty Goo <[email protected]>
Date:   Fri Mar 2 11:33:19 2018 -0500

    fix(text-field): disable validation check in setRequired (#2201)

    BREAKING CHANGE: removed setRequired and isRequired from foundation.

commit c14d244
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 10:37:18 2018 -0500

    chore(select): Fix ID values in demo (#2330)

    Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL.

commit ecf4060
Author: Bonnie Zhou <[email protected]>
Date:   Thu Mar 1 16:38:41 2018 -0500

    feat(chips): Change chip color when selected (#2329)

    BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively.

commit 4b24b51
Author: Matty Goo <[email protected]>
Date:   Wed Feb 28 15:20:19 2018 -0500

    chore(floating-label): separate label module from text-field (#2237)

    BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label`

commit fd8d8d9
Author: Will Ernest <[email protected]>
Date:   Wed Feb 28 14:21:55 2018 -0500

    feat(top-app-bar): Implement short top app bar (#2290)

    Adds the short top app bar variant to the top app bar.

commit a9f9bf2
Author: Kenneth G. Franqueiro <[email protected]>
Date:   Wed Feb 28 13:06:41 2018 -0500

    docs(select): Fix front matter for label sub-component (#2323)
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.

4 participants