primer / octicons

A scalable set of icons handcrafted with <3 by GitHub
https://primer.style/foundations/icons
MIT License
8.31k stars 827 forks source link

remove fill-rule="even-odd" #876

Closed gavinmn closed 1 year ago

gavinmn commented 1 year ago

Related to https://github.com/primer/octicons/issues/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
CleanShot 2022-12-01 at 15 23 58@2x CleanShot 2022-12-01 at 15 24 27@2x

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 https://github.com/primer/octicons/issues/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[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: 1e0fdff1f0a4bf96fb03240b52f534bf70391b3f

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 commented 1 year ago

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 commented 1 year ago

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 — https://github.com/primer/octicons/pull/876/commits/1e0fdff1f0a4bf96fb03240b52f534bf70391b3f#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 commented 1 year ago

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

gavinmn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Gonna go ahead and close this in favor of @eliperkins's PR: https://github.com/primer/octicons/pull/880