Skip to content
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

preserve attributes in css/scss/less output #121

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

fgiust
Copy link
Contributor

@fgiust fgiust commented Sep 26, 2020

see #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
Copy link
Owner

cascornelissen commented Sep 27, 2020

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
Copy link
Contributor Author

fgiust commented Sep 27, 2020

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
Copy link
Owner

@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.

@cascornelissen cascornelissen merged commit 69e5bbc into cascornelissen:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants