remove fill-rule="even-odd"

[gavinmn]

Related to #866

I was able to use https://github.com/googlefonts/picosvg to fix the fill-rule property of all of our Octicons. One thing to consider tho is that it looked like picosvg optimized strictly for filesize and the resulting icons have different point placements which makes the icons appear no different to the user, but theoretically would make editing an SVG from the library a bit clunkier. An example:

Pre-optimized icon	Fixed icon

Would love to hear some thoughts on these changes and if we want to consider adding something like this script to all icon pull requests to automate making sure we don't use fill-rule="evenodd". An alternative discussed in #866 was to add a CI test to catch evenodd which would require manual changing via the script or a Figma plugin before opening the pull request.

cc @lesliecdubs @eliperkins @tallys

[changeset-bot]

⚠️ No Changeset found

Latest commit: 1e0fdff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[colebemis]

This is great, @gavinmn! Could picosvg be a drop-in replacement for svgo in our optimize.yml workflow? https://github.com/primer/octicons/blob/main/.github/workflows/optimize.yml

We run that workflow on every push and commit the changes (if there are any).

[gavinmn]

I don't know a ton about SVG optimization, but I don't think picosvg is doing as good a job as svgo? See the difference in this diff — 1e0fdff#diff-bbeeefbb309f5267472a458b4fe7e89a4f11ecba103fd5ccfc8616f823a199e5

Left is the picosvg output which is useful for removing evenodd but on the right, the svgo optimization on top of that looks much better. To be fair tho, I don't totally know my way around picosvg and there might be more options to get better results! Does svgo have any options to remove evenodd during optimization?

[colebemis]

@gavinmn Ah, I see. We could run them both in that workflow: do the first pass with picosvg, then run svgo.

[gavinmn]

I'll dig further into this utility and see if there's any way to avoid moving / adding vector points. I think otherwise if we are in agreement that having these slightly funky svg point layouts is not an issue we could add it to the workflow to avoid any fill-rule issues.

[gavinmn]

From what I can tell, neither plugin would get us everything we want in isolation —picosvg doesn't result in great compression alone, and it doesn't look like any svgo plugin would remove / swap the error causing fill-rule. Are there any technical tradeoffs that would need to be made to run both? We can fix this issue and then make sure we don't use evenodd in Figma with a CI test to check for it as an alternative if implementing picosvg would cause problems.

[colebemis]

I think otherwise if we are in agreement that having these slightly funky svg point layouts is not an issue we could add it to the workflow to avoid any fill-rule issues.

I'm not worried about adding extra SVG points. These SVGs are the output of "outline stroke" in Figma, so they're already not the "source" file.

Are there any technical tradeoffs that would need to be made to run both?

I don't see any downside besides that it will take longer to run, which I'm not concerned about.

Go ahead and add picosvg to the workflow 👍

[gavinmn]

Going to work with Cole on getting this added as I'm not entirely sure how to add a python package to the workflow.

[eliperkins]

Happy to mob on this with y'all too, should you need an extra set of eyes! I'm no Python expert, but can lend a hand if it'd be helpful ❤️

[colebemis]

Gonna go ahead and close this in favor of @eliperkins's PR: #880