Added github comment icon workflow

[jguddas]

This automatically creates and updates a comment in PRs that shows a preview of all icons that have been changed or updated.

Example

Performance

The action itself is quick due to not running any calculations or even starting up Node.
The API route is cashed and takes less than 200ms to return the rendered svg.

Permission problems

This workflow only runs for PRs that are created from inside this repository.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

[karsa-mistmere]

This looks pretty neat, although useful as it is, I think we'll still have to do some validations manually.
I think the white "midlines" will need to be a bit thinner.
What exactly do the colours represent?

[jguddas]

The different colors are, so you can easily see when things are not 100% connected, like here in the cat ear for example.

[ericfennis]

@jguddas Nice work, looks really cool!
I think this really helps the review process of icons.
Maybe we can add the 100% scale variant of the icon somewhere as well.

I noticed that the builds are failing for site and the github action. Can you check what is going on?

[karsa-mistmere]

@jguddas: do you think it could be possible to different shapes for points within a path and endpoints of paths?
I think that could be super useful for finding misaligned paths and checking if paths are incorrectly merged or broken up for no reason.

[jguddas]

@jguddas: do you think it could be possible to different shapes for points within a path and endpoints of paths?

Great idea.

There are 3 things:

1. Path elements
2. Path segments (the things split in d using Move command)
3. Path commands.

I would like to highlight move commands by adding the big start/end circle.
What do you think?

I don't know how to best highlight the path elements.

• On the right side, the color is based on the path commands.
• On the left side, I changed it, so the colors are different for each Path element.

[karsa-mistmere]

Nice, the one on the right seems much more legible to me apart from colours that are a bit too close in hue.
A few more questions:

• Why is the top control point from the head's circle missing? Is it a bug or is it on purpose?
• Is it maybe possible to differentiate closed and non-closed paths? (again, for the head's circle it is not important which point is the starting point, since it's closed)

[jguddas]

@ericfennis I don't have access to the build, but I can tell you why the action is failing.

The action fails due to being run from a fork, I will see if I can find a quick fix.

[jguddas]

Why is the top control point from the head's circle missing? Is it a bug or is it on purpose?

It's due to being a circle element instead of a path, what do you think of only showing control points for path elements?

Is it maybe possible to differentiate closed and non-closed paths?

Good idea, I think it would definitely be nice to only show the start/end node for open paths.

[jguddas]

• I have removed the start and end marker for closed paths.
• I have removed the control points from non path elements (see head).
• I have changed it, so the control points are only shown for start and end of path segments.
• I have submitted a pr to svgo to optimize closed paths feat: remove useless closing path segments svg/svgo#1751
• I have submitted an issue to svgo to optimize arcs (like here in the wheel chair wheel) [convertPath]: combine arcs and convert full arcs to circles svg/svgo#1748

[jguddas]

I moved the whole thing into a react component 🎉

[jguddas]

That should be everything.

[ericfennis]

@jguddas Nice this looks event better!
Nice work. Good to go for me

[karsa-mistmere]

@jguddas: Really nice, I love the latest changes (dark mode support + extra 1px outline to check for gaps). :)

[karsa-mistmere]

@jguddas: I think this is almost good to go, but I still strongly believe CSS classes should be used to style the paths, since there are a looooot of repeating attribute values, and dark mode support should be handled not by using a <picture> tag and generating separate preview files for light and dark mode, but via CSS prefers-color-scheme media queries using the very same CSS classes.
(It could also be a good idea to insert HTML comments before each section, stating the purpose of that group of paths (grid, centerlines, outlines, points, arcs etc.).

[jguddas]

It would be nice to not have to use the picture tag, but I'm not sure how I would go about cleanly integrating style tags.

What about getting rid of the duplications by using groups like this?

- <p d="…" strokeWidth={5} />
- <p d="…" strokeWidth={5} />
+ <g id="grid" strokeWidth={5} >
+   <p d="…" />
+   <p d="…" />
+ </g>

[karsa-mistmere]

Yeah, grouping is also a good idea, although I'd probably still go for BEM style CSS classes instead of using the id attribute, should the SVGs ever need to be embedded in an HTML page (some debug page on the app review site maybe?) where the global IDs would no longer be unique.

[karsa-mistmere]

@jguddas: I've posted a PR for this branch that solves dark mode support via CSS only, eliminating the need for building separate icons for light and dark modes: jguddas#1

[karsa-mistmere]

@jguddas: any chance of also adding a curvature preview in v2.0? 😅🤪

as in https://www.figma.com/community/plugin/932729267119671976/Curvature

[jguddas]

An edit page that makes it easy for people to contribute by showing them errors and a preview of their icon would be super nice IMO.

[jguddas]

Okay @karsa-mistmere, @ericfennis lets merge this!