Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Extend styles with color mixin #42

Closed
wants to merge 1 commit into from
Closed

Extend styles with color mixin #42

wants to merge 1 commit into from

Conversation

Nodarii
Copy link

@Nodarii Nodarii commented Oct 8, 2015

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Nodarii
Copy link
Author

Nodarii commented Oct 8, 2015

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@samccone
Copy link
Contributor

samccone commented Oct 8, 2015

LGTM thx a ton @Nadarik

CC @notwaldorf

@@ -89,6 +100,7 @@

width: var(--iron-icon-width, 24px);
height: var(--iron-icon-height, 24px);
color: var(--iron-icon-color, inherit);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the color was being given by the fill: currentcolor; style right above. Is that still needed? Do they work together?

Copy link
Author

Choose a reason for hiding this comment

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

@notwaldorf, it's not the same.
It works with fill: currentcolor.
Right now we can colorize icons only via some color inheritance or by applying some global class like .green { color: green }. But with this change we will be able to apply isolated(encapsulated) custom-styles just like we do it with size (on the L28)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've merged #39 which adds custom properties for fillColor and strokeColor. Is this still needed?

Copy link
Author

Choose a reason for hiding this comment

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

Cool, it's not needed anymore! Thx

@notwaldorf
Copy link
Contributor

Closed as already fixed in a different PR

@notwaldorf notwaldorf closed this Oct 30, 2015
@Nodarii Nodarii deleted the patch-2 branch January 22, 2016 10:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants