Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

svg lettericons #55

Merged
merged 6 commits into from
Feb 28, 2021
Merged

svg lettericons #55

merged 6 commits into from
Feb 28, 2021

Conversation

gurgeous
Copy link
Contributor

The server can generate SVG lettericons. Only enabled if ?formats= includes svg. Most of the action is in RenderSVG. Also tweaks to IconPath to support SVG. For example, an SVG lettericon path doesn't include the size in the filename.

One thing to note. The A-Z lettericons look great (see below), but ф is a bit low. Right now we have dy="0.10em" hardcoded in the SVG template. Not sure if it would be possible/worthwhile to customize based on the letter. I could also imagine using Freetype to measure the Arial glyph height... The traco 1M doesn't include any IDN domains, unfortunately.

image

Copy link
Owner

@mat mat left a comment

Choose a reason for hiding this comment

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

Well done, looks really great, let's tackle improvement of the non A-Z's later, this is a great starting point, thank you 👍 Really just one comment not the svg lettericon paths.

@@ -188,6 +201,21 @@ func TestGetLetterIcon(t *testing.T) {
assertIntegerInInterval(t, 1500, 1800, w.Body.Len())
}

func TestGetLetterIconSVG(t *testing.T) {
req, err := http.NewRequest("GET", "/lettericons/M-144-EFC25D.svg", nil)
Copy link
Owner

Choose a reason for hiding this comment

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

Do we even want to allow for the size to be in the path for .svg? I'm thinking the fact the we allow it is kind of ok by Postel's law on the one hand but could lead to misunderstanding one the other (we're not using this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let's turn off support for size entirely. We're not generating those with SVG so no need to parse them. Give me a sec here

Comment on lines +324 to +327
vars := map[string]string{
"$BG_COLOR": ColorToHex(bgColor),
"$FG_COLOR": ColorToHex(pickForegroundColor(bgColor)),
"$LETTER": buf.String(),
Copy link
Owner

Choose a reason for hiding this comment

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

Like the lightweight templating approach for this👍

@gurgeous
Copy link
Contributor Author

OK, check out the latest commit. I reworked ParseIconPath a bit to consume params one at a time. Also added some tests for bad paths to make sure things were working properly. Thanks mat! 😄

@mat mat merged commit 78d3335 into mat:master Feb 28, 2021
@gurgeous gurgeous deleted the svg-lettericon branch March 1, 2021 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants