devicon icon indicating copy to clipboard operation
devicon copied to clipboard

[FEATURE REQUEST] Automatically convert svg styles to attributes

Open Snailedlt opened this issue 3 years ago • 11 comments

I have searched through the issues and didn't find my problem.

  • [X] Confirm

Problem

Upon fixing some stuff (#1276) causing a bug in the react-devicons project, I noticed that the process of converting SVG styles to attributes can be automated.

Some projects using devicons, like react-devicons will fail if styles are used in the SVG icons. This has led to several bugfixes. See #1169

Here's a screenshot of the issue: image

Possible Solution

SVGO has a neat script (convertStyleToAttrs.js) to go through and convert all SVG styles into attributes. We could leverage this script to automate the process with one of our bots (maybe make a new one?).

This would be help ensure the SVG's work in the react-devicons project and potentially future projects with similar solutions.

Additional information

No response

Snailedlt avatar Jul 20 '22 23:07 Snailedlt

@devicons/supporter @devicons/ecosystem-react What do you guys think of this feature?

Snailedlt avatar Jul 20 '22 23:07 Snailedlt

Sounds great but somebody would have to implement it.

maltejur avatar Jul 21 '22 15:07 maltejur

Alternatively SVGO could also be directly integrated into react-devicons.

maltejur avatar Jul 21 '22 15:07 maltejur

Upon looking a bit further into it I think that convertStyleToAttrs.js does something entirely different. It converts a single style attribute on one element to multiple independent attributes on the same element. But our problem is with style elements, not attributes.

maltejur avatar Jul 21 '22 16:07 maltejur

This is what we need: https://github.com/svg/svgo/blob/main/plugins/removeStyleElement.js But it just removes the style element entirely

maltejur avatar Jul 21 '22 16:07 maltejur

I've gone ahead and added removeStyleElement.js to react-devicons: https://github.com/devicons/react-devicons/pull/14

maltejur avatar Jul 21 '22 16:07 maltejur

@maltejur Yeah looks like you're right!

I found what we need though. Instead of just removing the style element, we want to convert it to inline style attributes. This can be done with this script: https://github.com/svg/svgo/blob/main/plugins/inlineStyles.js

Description of what it does here: https://github.com/svg/svgo#built-in-plugins

Plugin Description Default
inlineStyles move and merge styles from <style> elements to element style attributes enabled

Snailedlt avatar Jul 21 '22 19:07 Snailedlt

I've gone ahead and added removeStyleElement.js to react-devicons: devicons/react-devicons#14

Great, but like I mentioned above, I think what we need is actually the inlineStyles.js script

Snailedlt avatar Jul 21 '22 19:07 Snailedlt

Hi, yes @Snailedlt is right, removing the style in svg elements will break lots of SVG icons.

The conversion from this inline style:

style="fill-rule:evenodd;clip-rule:evenodd;"

To this HTML attributes styling:

fill-rule="evenodd" clip-rule="evenodd"

is required to preserve / not break icons.

Personally, when I add new icons I always use the style HTML attributes, but some old icons still have inline style="".

BenSouchet avatar Jul 25 '22 16:07 BenSouchet

@BenSouchet I think you're misunderstanding the issue here (just like I was earlier). You're talking about the style attribute, and not the style element, which is where the problem lies.

Style attribute looks like this:

<path style={{"fill-rule:evenodd;clip-rule:evenodd;"}} d="some path here"

Style element looks like this:

<style id="someId" type="text/css">
.someStyle{"fill-rule:evenodd;clip-rule:evenodd;"}
</style>

Now the first (Style attribute) is okey. Looks dirty, but it doesn't cause any errors. The 2nd (Style element) however is what's causing the error.

@maltejur ended up fixing the error by first running inlineStyles.js to inline any styles that are possible to inline, and then running removeStyleElement.js afterwards to remove the style element. Note that the change was implemented in the react-devicons repo instead of i the devicons repo since that's where the bug occured.

@BenSouchet We could consider adding this to this repo too, but then we'd have to make sure it doesn't cause nay other errors. We should also consider (like you say) converting the inline style attribute into separate attributes with the convertStyleToAttrs.js script, but that's a different discussion, so feel free to open a new issue or discussion for that.

Snailedlt avatar Jul 25 '22 17:07 Snailedlt

@Snailedlt Ah okay my bad 🙂

I opened an issue but it's not a priority, nor a vital element.

BenSouchet avatar Jul 25 '22 18:07 BenSouchet