Skip to content

Add (and fix) SVG cleanliness tests#209

Open
kognise wants to merge 2 commits intocubing:mainfrom
kognise:regression-tests
Open

Add (and fix) SVG cleanliness tests#209
kognise wants to merge 2 commits intocubing:mainfrom
kognise:regression-tests

Conversation

@kognise
Copy link
Contributor

@kognise kognise commented Feb 27, 2026

As suggested in #208, I added some new tests:

  • viewBox now must be set and have the correct size.
  • Elements must not have fill or stroke attributes or style properties set.
  • SVGs must have a valid xmlns.
  • The set of attributes on the root SVG must equal exactly width, height, viewBox, and xmlns. (Rationale: about half of the SVGs previously had the enable-background attribute, which does nothing. A small percentage had no viewBox, which causes rendering problems when using the raw SVGs as icons in some cases.)
  • Elements must match a whitelist: svg, g, path, circle, defs.

I edited a lot of icons to fix the tests, but I didn't modify any of the icons except to remove an incorrect white border from penalty-10e3.

Moved to #210: I also added the SVG source files to the NPM package files list. This makes it possible to import the icons as, for example, React components without needing framework-specific support. Happy to undo this if desired.

@kognise kognise force-pushed the regression-tests branch 2 times, most recently from 185c0bb to 04786e3 Compare February 27, 2026 17:49
Copy link
Member

@jfly jfly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The changes here look fine to me.

My only concern about getting stricter with our tests is losing out on the ability for non-technical people to contribute. Would a file created or edited by inkscape satisfy these new rules, or would it require manual (or svgo) optimization?

@kognise
Copy link
Contributor Author

kognise commented Feb 27, 2026

My only concern about getting stricter with our tests is losing out on the ability for non-technical people to contribute. Would a file created or edited by inkscape satisfy these new rules, or would it require manual (or svgo) optimization?

I believe files created entirely in Inkscape will, by default, contain a lot of extraneous tags that won't pass the tests. However, my opinion is that these SVGs should be cleaned up before being merged either way. I believe existing files edited in Inkscape as well as files created in Figma will be fine.

Cleaning SVGs is very easy to do with a tool like SVGOMG (which I use extensively even as a technical person) — you can just drag and drop or paste an SVG, and the copiable output SVG will pass all the tests as long as it doesn't have fill/stroke colors. Hardcoded colors may have to be edited out either way, I'm not sure there's a good way to get around this. We could recommend SVGOMG in the README if it makes contributing easier.

@jfly
Copy link
Member

jfly commented Feb 27, 2026

I believe files created entirely in Inkscape will, by default, contain a lot of extraneous tags that won't pass the tests.

Bummer, but I can get on board with this. It's a low-traffic repo, so we can help non technical people whose eyes would glaze over looking at CI logs. And worst case, we can always relax the constraints in the future.

We could recommend SVGOMG in the README if it makes contributing easier.

Yes, please add that.

@kognise kognise marked this pull request as ready for review February 27, 2026 23:52
@kognise
Copy link
Contributor Author

kognise commented Feb 27, 2026

Done, added a contributing section to the readme.

@kognise kognise requested a review from jfly February 27, 2026 23:55
Copy link
Member

@jfly jfly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a couple more tweaks! Thanks again.

If you're comfortable with git, feel free to rebase your commits before I merge, else I'll squash them when merging.

I'll wait a couple days before merging.

@jfly
Copy link
Member

jfly commented Feb 28, 2026

If you're comfortable with git, feel free to rebase your commits before I merge, else I'll squash them when merging.

I mean "feel free to squash the noise yourself into a nice story". You don't have to squash them down to one commit.

@kognise
Copy link
Contributor Author

kognise commented Feb 28, 2026

Rebased, commit history is cleaner now and fixed the package.json issue.

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