-
Notifications
You must be signed in to change notification settings - Fork 90
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
Improving Workflow #16
Comments
I am afraid that you might have misunderstood something here. How to use the generated SVG is explained here by Chris |
It would be nice if the output also included this block below so authors don't have to write it manually. You can obviously disagree here, but that's my thoughts so far. <svg viewBox="0 0 32 32" class="svg-icon">
<use xlink:href="#icon-menu"></use>
</svg> |
Ok I see your point. Maybe we have to think about a solution for that. I will look into a way of doing it and add a option for that. I don't really want that to be the default behaviour, because that would mean to increase the file size for all users. |
Awesome. Also these svg's were taken directly from the output icomoon gave which resulted in an odd layout behavior where I had to monkey with the |
Definitely don't include the I can see how that might be confusing... perhaps a |
Yup totally, but for demo purposes totally fine IMO. Just needs to be noted whether in an example file or a readme. Also could have sep output files (one for the defs and the other for the use blocks) If you look at my demo you will notice when using icomoon svgs you must set the viewport box size to what they were made at. In my case 32 x 32. I can't do the values '0 0 100 100' like you have in your demo. <svg viewBox="0 0 32 32" class="icon">
<use xlink:href="#icon-cloud"></use>
</svg> |
That's interesting. What happens if you do use |
Using those values |
I would really appreciate this, maybe run a mustache template You could add some basic css in the head to make all icons black and a certain size. |
Another use case to consider is that not all icons in the icon-system are the same size/aspect-ratio. The SVGs outputted by illustrator have a viewBox attribute on the tag itself, which of course, cannot be pulled into the defs. With large icon systems, it can become very cumbersome to remember which viewBox value goes with which icon (especially in large teams). My suggestion would be to use a data-attribute like solution, by adding the original SVG's viewBox data to every tag under the defs. Maybe using the desc attribute (http://www.w3.org/TR/SVG11/struct.html#DescriptionAndTitleElements). This way we can maybe use js to inject the correct viewBox sizes to the SVGs, or in the very least have a handy reference. |
I wonder if SVG allows data-* attribute on stuff like HTML does? Even if it doesn't, it probably wouldn't hurt anything.
|
That was my initial idea, but from what I managed to see, it's not in the spec (see this stack overflow, which is where I took the |
Using the namespaced attributes seems like a possibility but also possibly easy to screw up as an author because of the required extra attribute on the |
I will include an option to generate a separate |
I added a |
The demo html is nice to have, but I think the |
+1 for dynamic viewBox help. |
How would you like it to look? Just add a |
I'd say something like:
|
@TxHawks |
I think I have a better idea. Maybe the issue stems from But their big advantage is not the better semantics, but rather that they can hold many of the attributes and elements an svg tag itself can hold - including - its own, independent, Moreover, If I'm reading the spec correctly, the Am I missing something? To sum up, my proposal is that the structure be changed to:
|
Thank you for the research you are doing! It is great. I am using this structure now and it is working! Here is a pen with the proposed syntax. |
I can't see why you would want to change the viewBox in the |
My only addition to the proposed syntax in the pen, is also adding the original |
I am just passing the arguments from the source svg to the |
I just read the spec again and saw that |
Account for changes made during the implementation of #16
I pushed version |
Thanks for being so responsive on this. Your work is really great. I just tested it, and it seems to work perfectly. My only reservation is about the implementation of the fix to #1, but more on that there. Thanks again for all your work |
@chriscoyier, since your css-tricks article is probably the go-to guide on SVG icon systems, it may be a good idea to update it with the |
It might even be possible to drop the
So it is exactly what we want to do. |
Sounds about right... it would also mean that we can have a |
Right. But even than we need to use |
True, but still more semantic. If potential performance penalty in large and complex svg systems isn't too big, it's probably the best solution. Otherwise, two svgs, one for |
Okay, I changed this in |
Seems like a good change to me, especially with putting the viewBox on the |
Am I the only one that would like to see this line below outputted still? Ok I'm lazy, that's true, but I don't wanna type this out all the time. Copy and paste yo and not from a README or googling the syntax. Pleases and thank you's w/cherries on top. <svg><use xlink:href="#icon-cloud" /></svg> |
Use the |
Oh sweet! Definitely need to add that to the README even though I see it listed in the changelog. |
It is explained in the README: https://github.com/FWeinb/grunt-svgstore#optionsincludedemo-since-010 |
yep I missed it. I would still include a code block sample anyways. thanks again. |
It can't go in the final output file cause then that file isn't production-use ready, it's just a demo. I like how the demo is optional. Just my 2 cents. |
yay! 🎆 thanks @FWeinb and everyone else for the great collab to make this happen. |
So, I finally got the SVG sprite to work, but since the width and height attributes are not set dynamically one has to enter them in manually otherwise the icons sit at the bottom of the viewport. Output is below, but here's a pen to view also… http://codepen.io/grayghostvisuals/pen/f8a269745f17d11e8ae77bec37630170
The text was updated successfully, but these errors were encountered: