-
Notifications
You must be signed in to change notification settings - Fork 424
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
SVGs might be better not in the HTML? #103
Comments
We did it this way so that the color of the icons can be easily determined by CSS. As far as I know that isn't possible with sprites or referencing external files. While we currently only have each icon being white, users of this project could potentially modify any piece of the SVG with CSS. This makes the project much more customizable. Also, I don't think some extra text in the HTML file is contributing to any performance problems. It is just text after all. |
Hey @AdamPS, thanks for your thoughts! Rrssb is extremely lightweight. The inline svgs are minified. IMHO performance/size is a non-issue, especially considering the tradeoff. For now I'd prefer to stick with the svgs in the html for a few reasons:
If you still feel strongly about an "svgs as images" alternative, feel free to fork the project, and we'll be glad to link it up in the readme. |
As I recommended in another issue, maybe try using SVGInjector, then you can use AJAX requests to get your SVGs and it is quite a small and simple js library :) |
@dbox Would you like to see this in 2.0? I am putting the html in the js file but I could use ajax to load the icons? What do you think? |
I think it'd be much easier to implement without having to deal with images and image paths, plus we lose some of the customization. |
So you think we should hard code them in rather than ajax loading them? By the way when I say ajax load I just mean that you would put in the options iconLocation: "/images/rrssb/" for example, and then the icons would be in that folder and the script would load them from the icons folder. If you want to change the svg you can just alter the svg file and it will be loaded with the changes next time |
I know but you can't recolor them via css if they are linked images ...
|
You can, you just inject the raw html so it is svg elements If you just use image tags with the image path as the src then you can't though |
But maybe this is super rare use case. And if someone wants to recolor then
|
Fair enough, I'll leave it out then? |
May need to hear @joshuatuscan's thoughts, but I think I might be having a change of heart on this issue based on the new plugin style call referenced in #49. In the current setup, you manually include or exclude The 2.0 setup reverses this though. If svgs were hard-coded, the JS would have a lot of unnecessary icon bloat right? Vs images which would only be called if needed. I also think ~99% people use the defualt icon settings, so losing the ability to recolor the actual icon via scss is maybe not the worst thing. (Someone could still do it via the actual icons.) And everything else is still customizable. |
Yeah you're right, the icons would have to be in the js file if they arent loaded by ajax. You wouldnt lose the ability to style icons though by using ajax to include them, they would still be svg elements in the dom rather than an img tag. On my site http://connorwyatt.io, which has just gone up so I can show you, the logo in the top left next to Connor Wyatt is injected via ajax and if you add look at the css, the colour is determined by a css rule |
Wait, you're saying you can ajax-load the inline svgs? |
Yeah exactly :)
|
So in this scenario:
Correct? |
Or you'd still have to have the image folder, but the ajax would just be referencing it and writing it out as inline? |
Exactly because you can:
|
Yeah exactly but you could have a CDN if you wanted because github allows you to access the raw files like this: https://raw.githubusercontent.com/kni-labs/rrssb/master/icons/facebook.min.svg |
You would have to keep the icons the same though or the users' links would break |
Up to you which you think is best |
I don't know whether that would be disallowed due to same domain issues though? |
So, one of my main arguments before to not linking to the SVGS, was, in the current setup you don't even need the images in your folder structure. But now, the tradeoff boils down to: Ability to change the white icon vs css Correct? |
Kinda but there is no tradeoff because you can still style them with CSS and you get the performance benefits |
But not the caching right? |
I think you would still get the caching because ajax has a setting to turn it off with jquery: http://api.jquery.com/jquery.ajax/ - Look for the cache setting |
Okay, perfect. |
Awesome I'll write that in then :) |
I'll use the 2.0 thread from now on |
Great, thanks guys! |
Recently there have been several articles with lots of clever ideas for other ways to handle SVGs. The key benefit I see is page-loading speed. I wonder if it would be worth improving RRSSB along these lines - well at least I can explain my reasoning and you can see what you think. It would not change the JS, would simplify the index.html, and move the corresponding SVG code to an SVG/CSS file.
Sorry this is a quite long...
Here are some articles describing alternative techniques:
The potential benefits in detail:
The articles I reference largely assume a requirement to be able to use CSS to style elements within the SVG. RRSSB appears to do that, but minimally (one line? {fill:#fff}). Maybe if that line were moved into each SVG, then the SVG files can be standalone. This way much simpler approaches would be possible, a bit like this site does: http://www.metoffice.gov.uk/public/weather/forecast/gcvwr3zrw. This is basically a bit like "SVG fragment identifiers", a little less neat, but much more compatible.
The text was updated successfully, but these errors were encountered: