search

cascornelissen / svg-spritemap-webpack-plugin

SVG spritemap plugin for webpack
MIT License
210 stars 51 forks source link

preserve attributes in css/scss/less output #121

Closed fgiust closed 4 years ago

fgiust commented 4 years ago

see https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/120

I've actually updated one existing test image to add a "fill" top level attribute to ensure that all the existing tests were covering this case. Generated svg files were always ok with the fill attribute moved from the top level to the symbol child element, I just updated the expected output to match it.

For scss/css/less output the input svg was took as is (without moving the fill attribute to child element) but also the root attribute was stripped. This patch fixes the css renderers in order to preserve any meaningful attribute.

cascornelissen commented 4 years ago

I'm wondering if we should add an option to enable this (maybe styles.keepAttributes which defaults to false) since this change might just break things for a lot of people because they are actually expecting the output without the attributes.

Any opinions on this from your side?

Also, code LGTM but I won't be able to merge/release this in the next 2 or 3 days because of personal stuff.

fgiust commented 4 years ago

hi @cascornelissen , in my opinion this should be the default, stripping attributes from the original svg can only lead to a broken result, I can't see why an altered svg should be expected. If you look at the sample that I added for test you will see that the output is a red image for all the file based renderers and a black one for css/scss without this patch.

We've been using a patched version of the plugin for about 8 months on at least 4 different projects with inputs from different design agencies (hundreds of svgs) and we never saw a problem with it.

cascornelissen commented 4 years ago

@fgiust, I agree with you but releasing this while it's not behind a flag will result in changes for users which they won't be expecting. I'll add the option for it and we'll change the default value to true in the next major release.