cascornelissen / svg-spritemap-webpack-plugin

SVG spritemap plugin for webpack
MIT License
207 stars 50 forks source link

css/scss renderers strip needed attributes from svgs #120

Closed fgiust closed 3 years ago

fgiust commented 3 years ago

when using the plugin for generating a scss with embedded svgs we often end up with broken images due to the fact that some information are stripped from the main svg tag. In particular it strips the "fill" attribute which often is set to "none", and without that svgs tends to have a black background.

here is a simple svg you can try with a screenshot of the broken result:

info-circle.svg.txt

brokenfill

here is a simple patch to style-formatters/scss.js for fixing this issue. Basically, instead of copying only the viewBox attribute, it just clones everything except for 'width', 'height', 'id' and 'xmlns':

svg-spritemap-webpack-plugin+3.5.10.patch.txt

cascornelissen commented 3 years ago

My initial response would be that you definitely need to manually optimize these SVGs as these symptoms come from tools like Sketch and Adobe Illustrator and the current SVG with fill="none" is highly likely to be bigger than what's actually needed to render the icon.

But if the patch you've provided fixes the issue for you I'm open to a PR that adds this logic in all of the available style formatters and includes updated/improved tests.

fgiust commented 3 years ago

thanks @cascornelissen , pull request ready: https://github.com/cascornelissen/svg-spritemap-webpack-plugin/pull/121

Apart from patching the scss/css/less renderers I've updated the existing by adding a fill attribute to a test svg and reviewing the output to ensure that the fill was considered.

Just FYI similar svgs are often generated when extracting vectors from Figma, we saw this problem in several projects that were using that tool.

cascornelissen commented 3 years ago

@fgiust, I've merged #121 and added the styles.keepAttributes option. Could you verify whether this works for you (keep in mind that you'll have to set the styles.keepAttributes option to true) by installing it from master?

npm install cascornelissen/svg-spritemap-webpack-plugin#master
cascornelissen commented 3 years ago

ping @fgiust

fgiust commented 3 years ago

confirmed @cascornelissen, installed from master without patches, I can see broken svgs with styles.keepAttributes=false and good ones with styles.keepAttributes=true 👍

cascornelissen commented 3 years ago

Good to hear, I'll include this in the next release 👍🏼

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

cascornelissen commented 3 years ago

Bad bot! I'll include this in release 3.6.0 which currently has the next dist-tag to ensure Webpack 4/5 support is working as expected.

cascornelissen commented 3 years ago

3.6.2 has been marked as latest on NPM now so this is released. Thanks once more for your contribution. 🚀

fgiust commented 3 years ago

wonderful, thanks @cascornelissen