search

FWeinb / grunt-svgstore

Merge svgs from a folder
MIT License
920 stars 94 forks source link

fill properties with a value of currentColor should be preserved #63

Closed roblevintennis closed 9 years ago

roblevintennis commented 9 years ago

@FWeinb First of all thank you for your effort and contribution with this library...saving us tons of time :+1:

We're using inline SVG here and it looks like the only way to allow for more than one color to be applied to the inline def by multiple use xlink elements in a unique way (e.g. I want some inner path, shape, etc., to have a different color for 2+ instances) is to take advantage of the currentColor CSS3 property. Thus, this PR checks if it's a fill and if it has currentColor, and opts to NOT remove those.

There's a nice codepen I found while researching this requirement that is likely more descriptive than my last paragraph: http://codepen.io/jjenzz/pen/AxLKz

Feedback: I hope you don't mind me giving feedback, but I found the test/expected/includedemo-demo.html particularly cumbersome since I was adding a new svg element. I had to force it in by calling grunt svgstore directly to get the file corrected, then copy over to the expected. Seems like it might be worth pruning out this expectation or making it somehow less fragile.

FWeinb commented 9 years ago

@roblevintennis Thanks for this! I just had a look at it and it's awesome :fireworks:

The includedemo test is bad i know. It's more like a end2end test than a unit test and will break in any change you make to the grunt-svgstore. But it is a good way to force you to check the resulting include-demo.html and make sure everything looks good.

FWeinb commented 9 years ago

Released in 0.3.6

roblevintennis commented 9 years ago

@FWeinb thanks! Yeah, I have a bunch of ideas that all fall short for the integration test. Somehow it seems like you want to 1. guarantee the file is regenerated and tried out 2. it would be nice to have it as a separate grunt task maybe.

Not sure on how to solve that