#svg

I've pulled over a few additional best practices from the current issue I was working on 📌 Update Drupal's default file type icons Active .

For anyone following along here, I've opened another issue related to PNG to SVG conversion in core file icons: 📌 Update Drupal's default file type icons Active

Re #58:

Default presentation attributes (fill/stroke/etc) should be moved off from SVG tags if it reduces the weight, and placed as Internal CSS

Please don't do this, especially not in core. I even propose to avoid CSS styles and encourage the use of presentation attributes, as they are very easy to overrule in CSS by contrib+custom modules and themes.

I tend to agree, but I feel like it is hard to make call one way or the other. It seems to me that this is highly dependent on how complex the SVG (or group of SVGs) is and how many attributes there are that would be shared/duplicated. Furthermore, there is a good argument that by using classes instead of inline attributes, the classnames become the api, giving core the freedom to change/tweak things like hex colors getting converted to rgb or CSS attributes eg path.something {fill: var(--my-color, #00ff00); } without breaking the overrides in your custom theme.

To reframe my point in the context of your example on comment #58...

This:

#contrib-module a svg path.something {
  fill: blue !important;
}
#contrib-module a:hover svg path.something {
  fill: red !important;
}

Seems much less fickle than:

#contrib-module a svg path[fill="#00ff00"] {
   fill: blue !important;
}
#contrib-module a:hover svg path[fill="#00ff00"] {
   fill: blue !important;
}

In the second example, if the SVG from core or contrib ever changes the fill attribute value, your theme override breaks. I could be wrong here but it seems unexpected that the fill attribute value is the API, and OTOH, much more obvious that a classname is an API, and has been purposefully added in the SVG to make theme overrides more stable.