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
Break SVG sprite into individual icons #6
Comments
I didn't dig too much into how TwentySeventeen does this, and the reasons for it, so let me know if you think there are reasons to keep it. |
There is a reason. It's not about having a sprite, but having an inline SVG. This will give you the power to use CSS to change the Icons, like giving them a new color without the need to provide different sprites for different colors. |
Something like this: #icon-twitter {
fill: #1da1f2;
} |
I don't understand why that needs a sprite, though. Couldn't you also just do |
That would be possible, but the benefit of having the icon in a Here is a talk I have seen in London last year, explaining the topic: http://wordpress.tv/2016/05/28/sarah-semark-stop-using-icon-fonts-love-svg/ I think that's the reason the |
I did some digging around, but haven't found anything saying why
Are there rendering performance concerns with having multiple inline |
Oh, I see, you meant inline That makes sense to me. I'm still not sure that sprites are needed/worth it, though. Couldn't we just have something like this:
Is the problem there with IE support? |
The problem with IE still exists. The current IE11 can handle inline SVG symbols but not external (as mentioned in the ressources you've linked). I created a small test for that, if you want to checkyourself: https://kau-boys.com/stuff/svg-symbols/svg-symbols-test.html So I agree, that it might add some markup to the HTML (about 1K gzipped), but that shouldn't be a real issue. It does increase browser compatibility and does not require any extra work for things like dynamically loading more icons while working in the customizer. I would say the TwentySeventeen team thought about it a lot and found the best solution ;) |
My main concern isn't the size of HTML document, it's the added complexity and maintenance cost, especially if we ever want to customize it. It just seems like a lot of cruft for something that should be simple. It even includes a fallback for IE8... I doubt that TwentyEighteen will do that. I think a cleaner solution for IE11 support might be a polyfill, like svgxuse or svg-use-it. At first glance, svgxuse seems like the better of the two, since it's more people using/testing it, and has been updated more recently. I don't mean any disrespect to the TwentySeventeen team, they're all talented people. Just because they chose a particular solution for their goal doesn't mean that's the best solution here, though. It's also entirely possible that there are other acceptable solutions. Nothing is perfect, everything has tradeoffs, and different people value those tradeoffs differently. I don't think using the current sprite technique is a blocker, but it does seem like unnecessary complexity. I'm still open to going w/ the sprites, though. @coreymckrill, do you have any thoughts one way or the other? |
Would customizing it be anything other than adding new icons? What would be the process for adding a new icon? I'm not sure I understand how having one file with multiple SVG sprites is more complex than having several individual SVG image files. But maybe I'm not understanding the issue... |
@2ndkauboy can correct me if I'm wrong, but I think adding new icons would just be a matter of adding them to the sprite, so that's not really a problem. I guess it's more the Imagine if every time you wanted to add a If I haven't convinced either of you, though, then maybe I'm making too much out of it. I don't think it's a blocker either way, so II'll go ahead close this out. |
WordCamp.org supports HTTP2, so spriting images doesn't significantly help load times. Without the performance gains, I don't think the added complexity and maintenance is worth it.
The text was updated successfully, but these errors were encountered: