Explicitly specify width and height for svg

[CirnoT]

Lot of octicons are not ideally 16x16. The issues were mostly invisible but they could be noticed for fold icon having improper positioning. What made it very visible is recently introduced internal icon which looks awful with 16px width.

Refactored svg template helper to accept width and height explicitly and changed most visible octicons according to data from https://primer.style/octicons/

[CirnoT]

[silverwind]

I guess this is necessary for proper rendering of non-square SVGs but I wonder if there is maybe a more elegant solution to letterbox the icon within size pixels.

Do we know how many of the octicons are non-square? I imagine it would be only a small percentage.

[silverwind]

Some non-square octicons can be observed in the logo of https://gist.github.com/. It does look like GitHub also just specifies both width and height there.

[CirnoT]

A good chunk of them are 14x16, just check commits. We also use variants where we increase size, so there most likely isn't any more elegant solution. GitHub also specifies them explicitly.

[CirnoT]

The new ones are explicitly 16x16 always https://primer.style/octicons-v2/ but are missing some icons we use and marked as not ready.

[silverwind]

Their JSON file does have the dimensions in them:

$ cat node_modules/@primer/octicons/build/data.json | jq '.["internal-repo"].width,.["internal-repo"].height'
13
16

So I guess once we tackle #11618, we can just use their defined dimensions. I'd then make the width/height attributes optional in the template helper and default them to the defined size. Or update to v2 and assume 16px.

[CirnoT]

I'd then make the width/height attributes optional in the template helper and default them to the defined size. Or update to v2 and assume 16px.

Sounds good.

[silverwind]

Still, I think it's rather inconvenient that one has to look up the icon size every time one needs to be added. Do you think it'd be much work to integrate the data from that json file and make the width/height arguments optional via vararg?

[CirnoT]

I'd say parsing JSON would be 3/4 of work required for #11618.

[silverwind]

I'm not sure of this is really worth it. For the repo icons, it would introduce inconsistent widths for the icons which can break horizontal alignment. Here's one icon at 16px, another at 13px width.

The increased width adds 1.5px whitespace left and right which I think is the best possible rendering and I don't think it looks awful at all. I tested this in Firefox and Chrome, both behaving similarily, not sure exactly what causes the blurriness in your example above.

Even if I add excessive width like 32px, it seems to correctly letterbox the icon for me:

[CirnoT]

@silverwind This only happens when there's an icon with improper width along with icon with proper width. Both internal and normal repo icons have different width but your example only adjusted one internal icon.

If you care to examine PR in full you should find out that if all icons are adjusted to their proper width, there is no horizontal alignment issue.

[CirnoT]

They don't appear blurry to me when in zoom, but on 100% at 1080p they look blurry to me on Firefox.

[CirnoT]

I agree that it is rather annoying to specify width explicitly. The alternative is to work on JSON parsing and handle it automatically like it should've been.

Also personally along with JSON parsing I would get rid of optional width and height and only allow float zoom to be given as option.

[silverwind]

Maybe you can show a jsfiddle with what you mean with the two icon alignment? A quick test shows that SVGs always letterbox correctly for me. For example, if I force a width of 24px on the first SVG on https://gist.github.com/:

The only way I found to break the scaling is by adding a preserveAspectRatio="none" attribute:

[CirnoT]

I don't understand what you mean. You raised issue that two icons were horizontally mis-aligned for you when you adjusted width on one of them and I corrected you that if you properly specify correct width for all of them, then they are horizontally aligned properly (even if they have different native width like lock and repo icon)

[CirnoT]

Example of what this PR fixes for me:

[CirnoT]

The specific case of this icon being blurry is because it's kind of special in whole octicon set - it's width is 13px. Setting it to 17px does not cause it to become blurry, but 16 or 18 does.

[lafriks]

would it be possible to move these sizes out to some kind of map in golang code that would svg function automatically use so that you don't have to specify all in template files. If icon name is not in map it would render default 16x16

[CirnoT]

Thank you Firefox, I love you.

shape-rendering: crispEdges

via https://bugzilla.mozilla.org/show_bug.cgi?id=1003763#c1

The issue was apparently fixed but doesn't seem to be the case for me; possibly due to a font variation or rendering mechanism, who knows. Original bug at https://bugzilla.mozilla.org/show_bug.cgi?id=608812

[CirnoT]

@lafriks Yes it's to be done whenever we get to handle #11618

[CirnoT]

For what it's worth, it's blurry on Chromium on Windows for me too.

[silverwind]

if you properly specify correct width for all of them, then they are horizontally aligned

How can they align with different width set on them? Is there another mechanism that corrects the width?