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

fill="currentColor" #3230

Closed
bhadaway opened this issue Jun 19, 2020 · 8 comments
Closed

fill="currentColor" #3230

bhadaway opened this issue Jun 19, 2020 · 8 comments
Labels
package Pull requests or issues that affect the NPM package won't add Icon requests or other features that won't be added

Comments

@bhadaway
Copy link
Contributor

Improvement Request

I'd love to see fill="currentColor" added to all SVG paths by default.

@bhadaway bhadaway added the package Pull requests or issues that affect the NPM package label Jun 19, 2020
@NovaGL
Copy link
Contributor

NovaGL commented Jun 19, 2020

Can I ask why? If you have a look at the examples you can use the HEX to get the correct color to use.

@bhadaway
Copy link
Contributor Author

This is for production purposes. I think many (most) in actual use, won't be using the brand color for each icon (while that's certainly useful information to have of course), but rather, in most designs, and for most other purposes, people will probably be using one solid color for all icons.

Example: https://user-images.githubusercontent.com/746834/85096702-a0ffcd00-b1e4-11ea-8f1c-83adcae817a6.png

I'm not sure if this will be of any use to anyone, but I personally also like to explode my svgs with jQuery, for greater CSS control:

$("img.svg").each(function () {
var $img = $(this);
var imgURL = $img.attr("src");
var attributes = $img.prop('attributes');
$.get(imgURL, function (data) {
var $svg = $(data).find("svg");
$svg = $svg.removeAttr("xmlns:a");
$.each(attributes, function () {
$svg.attr(this.name, this.value);
});
$img.replaceWith($svg);
}, "xml");
});

In any case, for most people fill="currentColor" should be safe and will have no effect at all, but for those that need to manually make adjustments, it could be helpful, even for people that want to use the brand colors.

@ericcornelissen
Copy link
Contributor

Thanks for the request @bhadaway, however I'm not sure if I fully understand. If what you want is to color all icons in some container the same colour, that can be easily achieved with some CSS. Could you elaborate how you are using the SVGs of this project (NPM package, raw SVG, CDN, other?) From the example you give it seems that you're using the icons through an <img>, if that is correct, what link do you use?

Furthermore, from the example it seems that you should be able to use Jquery to apply a color. Is this solution not good enough? What is it lacking?

Lastly, we are not in a position to assume the use case of the icons and I don't think we can just add fill="currentColor" as it relies heavily on the underlying CSS and can easily have unintended consequences (as demonstrated by this artificial example). At best we can make a change like this with a major release. That said, I do hope we can find a suitable solution.

@bhadaway
Copy link
Contributor Author

I just download the icons manually, insert fill="currentColor", and then add them to projects as normal images:

<a href="https://twitter.com/example" title="Twitter" rel="me" target="_blank"><img src="https://example.com/images/twitter.svg" alt="Twitter" class="svg" /></a>

I build a lot of tools like WordPress themes, plugins, website templates, etc., where the ultimate goal is simplifying, automating, and future-proofing things as much as possible. The biggest benefit of fill="currentColor" is that it'll automatically inherit the link color, and the link color often reflects the brand color, which I think most people will be after.

Essentially, it makes the icons more versatile, in that they're automatically styled to be consistent with the other content in which they're embedded. Sure, you can always style the SVG with CSS manually as needed; that's true no matter what. This just offers some assistance.

Assuming that even without it, most people are going to colorize the icons to some extent, probably the only people that would be annoyed are those that just want them in plain black (I would imagine that's a small %). Like anything though, it's a balance; you can never please everyone; you just hope to please the majority.

I'm sure there are some quirks that might not be desirable in some cases, but your example wouldn't be valid I don't think, because the issue of someone styling white text on a white background is already inherently wrong and wouldn't work, causing a much worse issue with their design than anything concerning the icons.

All that said, I have no idea if this is good addition or not. It'll certainly benefit me, but I just wanted to put the feelers out and see what others thought.

@NovaGL NovaGL added the in discussion There is an ongoing discussion that should be finished before we can continue label Jun 21, 2020
@ericcornelissen
Copy link
Contributor

I build a lot of tools like WordPress themes, plugins, website templates, etc., where the ultimate goal is simplifying, automating, and future-proofing things as much as possible. [...]

In that case I would recommend the WordPress plugin created by one of our community members: https://wordpress.org/plugins/simple-icons/

Assuming that even without it, most people are going to colorize the icons to some extent, probably the only people that would be annoyed are those that just want them in plain black (I would imagine that's a small %). Like anything though, it's a balance; you can never please everyone; you just hope to please the majority.

That is a fair point, though again from our perspective we cannot simply just change this without a major release (which we preferably do as little as possible) to actually realize it. I'm sorry if my comment came of as: "We won't implement this, case closed." I was 1) not sure how you're using the icons so not able to provide an answer particular to your problem and 2) looking for an alternative solution.

I'm sure there are some quirks that might not be desirable in some cases, but your example wouldn't be valid I don't think, because the issue of someone styling white text on a white background is already inherently wrong and wouldn't work, causing a much worse issue with their design than anything concerning the icons.

Again, it's an artificial example to show that it might have unintended side-effects. It's impossible for us to know how people are using our icons. Perhaps the icon is in a container with a black background but it overflows the container on a white background?

@bhadaway
Copy link
Contributor Author

Concerning WordPress in general, a WordPress plugin might be useful to individual end-users, but not in the context of what I'm doing, building custom themes with their own built-in options.

No reason to apologize; I didn't read anything in your response as flippant, nor did I respond in any way that would express that I felt so. Again, I want to emphasize that I have no idea if this is a good idea that would benefit many others or not. It would benefit me (and only slightly mind you) and if it would only benefit me and a handful of others then it's definitely not a good addition, and I have no expectations of it being included. It shouldn't.

I've merely posed the suggestion, and if others think it's useful, maybe it'll happen, but I completely agree about feedback; it's impossible to know what the majority think. I suffer the same problem with my own projects. You can ask for feedback all day long, and at best, maybe you'd get a small % of users to respond.

That leaves you with some guesswork, and an abundance of caution is definitely an excellent approach. Especially if you have a lot of users. The more users you have, that responsibility becomes exponentially important. I've seen projects make the tiniest of adjustments, but because they had hundreds of thousands of users or even millions, the backlash was extreme.

My guess is that if you added it, the majority wouldn't notice any difference one way or the other, some people would benefit from it, and not even realize why, even less would actually understand the change and appreciate it, and then definitely, you're right, some unknown edge cases are bound to happen where people respond:

"What the hell, you broke my icons!"

I think I recall the origin of why I first adopted fill="currentColor". It was concerning the debate between icon fonts and svgs. I chose svgs, but it was then about making svgs as flexible and adaptive as possible.

Some general thoughts out in the wild:

https://www.lambdatest.com/blog/its-2019-lets-end-the-debate-on-icon-fonts-vs-svg-icons/
https://css-tricks.com/cascading-svg-fill-color/
https://css-tricks.com/gotchas-on-getting-svg-into-production/
https://iamsteve.me/blog/entry/lets-make-a-better-icon-system-with-svg

Anyway, I've made my suggestion and the best case for why this might be useful (I'm not confident that I made a very strong case), and I'll leave it in your hands now. Hopefully others in the community will chime in to whether they like/dislike the idea.

If you decide to close the suggestion, I'll respect that decision. Please don't worry about me feeling dismissed. Having over a decade of experience with customer/user support myself, I understand that you deal with some sensitive/confused/angry people sometimes, but that's not the case here, so no worries. :)

Thank you!

@PeterShaggyNoble
Copy link
Member

"What the hell, you broke my icons!"

Yeah, that's enough for me to say we shouldn't add it; we can't foresee every possible way people will be consuming our icons so we need to keep things to the "lowest common denominator" to avoid breaking anyone's implementation. There are other icons libraries that have added this attribute, either to the <svg> or <path> tag, and encountered problems with it.

@PeterShaggyNoble PeterShaggyNoble added won't add Icon requests or other features that won't be added and removed in discussion There is an ongoing discussion that should be finished before we can continue labels Jun 22, 2020
@bhadaway
Copy link
Contributor Author

There are other icons libraries that have added this attribute, either to the or tag, and encountered problems with it.

Good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package Pull requests or issues that affect the NPM package won't add Icon requests or other features that won't be added
Projects
None yet
Development

No branches or pull requests

4 participants