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

Automatically build fonts in github-actions CI with FontCustom #158

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

lucjaulmes
Copy link

@lucjaulmes lucjaulmes commented May 7, 2021

Fixes #109.
Fork-Awesome uses FontCustom to build their fonts. This PR allows doing the same with academicons.

  • On every commit, the following files are built:

    academicons-preview.html
    css/academicons.css
    css/academicons.min.css
    fonts/academicons.eot
    fonts/academicons.svg
    fonts/academicons.ttf
    fonts/academicons.woff
    fonts/academicons.woff2
    

    That is, the same organization of files as the current repo, with an additional preview file.

    All these files are uploaded in a single academicons.zip archive downloadable from the “artifacts” drop-down menu of the github actions.
    artifcats menu screenshot

  • Visual inspection of fonts is possible by extracting the archive and opening academicons-preview.html

  • PRs only need to add an svg file to the svg/ directory with the desired basename. Fonts are automatically built, see for example the test PR Add ERC icon as a test PR lucjaulmes/academicons#1 which triggered the build:
    https://github.com/lucjaulmes/academicons/pull/1/checks?check_run_id=2527748877

    Adding erc.svg correctly adds a .ai-erc icon. Opening the preview file from the artifacts confirms my ERC icon from European Research Council (ERC) icon request #105 looks terrible :)

    ERC icon preview

I’m not sure whether keeping the css and fonts in the source code remains useful after this PR ?

Copy link
Collaborator

@nfrerebeau nfrerebeau left a comment

Choose a reason for hiding this comment

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

It looks pretty cool!

At first glance there are a few problems though:

  • Some settings need to be changed to match the current font properties:
font_design_size: 16
font_em: 512
font_ascent: 448
font_descent: 64
  • Many projects use academicons through the jsdelivr CDN which distributes the repository content. Instead of downloading the artifacts, the output of FontCustom should be deployed to the master branch after inspection. Artifacts are retained for 90 days before they are automatically deleted.
  • FontCustom's character encoding is different from the current encoding (e.g. Academia: \f100 vs \e9af). Maybe this can be defined in the FontCustom settings (at least so that the existing icons keep the same encoding)?

The first two items are easy to fix, but I have no clue for the third.

@lucjaulmes
Copy link
Author

I’ll look into getting the right encoding. Are the codepoints per icon defined somewhere ?


Not sure what the best way for jsdelivr though, I didn’t realize it just mirrored files from a github repo directly.

This script now builds all the files that need to be deployed (+ 1 preview file). Maybe a different branch could contain the sources, and pushing to master could become equivalent to publishing (with this workflow doing that) ?

Otherwise there’ll be double commits to the master branch: 1 updating sources + 1 auto-generated to update fonts and css. That would be pretty annoying, your remote refs would be invalid every time you push, the workflow should take care not to trigger on its own commits to master, etc etc.

@lucjaulmes
Copy link
Author

Updated the codepoints. It’s not the most user-friendly system to modify the (hidden) json manifest manually, so I made a config/codepoints.json that defines the codepoints and that automatically generates the manifest correctly.

I think (under your control @nfrerebeau) that that handles generating the fonts correctly. Easiest way to test that would be to generate that table again, but I’m unsure how to do that. We can further tweak css generation with templates.

Before looking at how this could be deployed, I think the next question should be what tests to include ?

  • It would make sense on PRs to check that added codepoints are in the correct range.
  • Not sure how to test “visually” that the font generation went well ?

@nfrerebeau
Copy link
Collaborator

I will look at all this in the next few days.

@lucjaulmes
Copy link
Author

This last commit comes from the fact I added a final (I think) feature:

  • automatic deployment is not done when there are too many pixels changed at once. This allows to prevent deploying breaking changes.
  • If there is ever a need for a push where lots of pixels change and it is normal (lots of icons updated, different browser used to render the screenshot, something like that), there is now a way to manually trigger the workflow with a 'force' flag to force ignore the pixel diff count

Copy link
Collaborator

@nfrerebeau nfrerebeau left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. The workflow seems OK, but there is an issue with the glyph generation.

If auto_width is false, all glyphs are square and not centered. If auto_width is true, the glyph width is adjusted to the width of the svg path with a maximum width of 512px:

  • We must be able to keep this white space for aesthetic reasons (we cannot rely on a CSS trick due to non-web uses of the font).
  • Icons larger than 512px are scaled back (e.g. Zenodo): the baseline is moved and the overall consistency of the font is lost.

Unfortunately this seems to be a limitation of FontCustom: FontCustom/fontcustom#323... Also FontCustom seems to be unmaintained (but this may not be an immediate problem).

@lucjaulmes
Copy link
Author

lucjaulmes commented Jun 14, 2021

Sorry, I fail to see how the generated fonts with audo_width=false are different from the current fonts.

I tested the differences between both fonts without using the generated css or svg files. I fetched the fonts from https://cdn.jsdelivr.net/gh/jpswalsh/academicons/fonts/ under the name academicons-legacy and the fonts from https://cdn.jsdelivr.net/gh/lucjaulmes/academicons/fonts/ under the name academicons-fontforge, and displayed them side-by-side by using the codepoints in HTML, and not the CSS classes (so definitely using fonts and not CSS images). I’ve also padded with underscores to visualize the baseline.

Both fonts look identical to me:
image

Some glyphs are wider than ohers, e.g. zenodo, but it’s the same in both fonts. You can see the ACM and all square fonts correctly dip below the baseline.

Here’s the file used to generate this image (gzipped to allow upload): diff.html.gz

@nfrerebeau
Copy link
Collaborator

My bad, I manually inspected the fonts with FontForge and it seems to be related to the import in FontForge.

@lucjaulmes
Copy link
Author

So this works right? Is there anything else you’d like included/changed?

lucjaulmes and others added 15 commits November 10, 2023 14:06
Also update from deprecated set-output and artifact up/download action versions
Bumps [commonmarker](https://github.com/gjtorikian/commonmarker) from 0.23.9 to 0.23.10.
- [Release notes](https://github.com/gjtorikian/commonmarker/releases)
- [Changelog](https://github.com/gjtorikian/commonmarker/blob/v0.23.10/CHANGELOG.md)
- [Commits](gjtorikian/commonmarker@v0.23.9...v0.23.10)

---
updated-dependencies:
- dependency-name: commonmarker
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [activesupport](https://github.com/rails/rails) from 7.0.5 to 7.0.7.2.
- [Release notes](https://github.com/rails/rails/releases)
- [Changelog](https://github.com/rails/rails/blob/v7.0.7.2/activesupport/CHANGELOG.md)
- [Commits](rails/rails@v7.0.5...v7.0.7.2)

---
updated-dependencies:
- dependency-name: activesupport
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
…/activesupport-7.0.7.2

Bump activesupport from 7.0.5 to 7.0.7.2 in /docs
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.

Distributing work of adding icons via PR?
2 participants