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

Current status revision #8

Closed
vv-monsalve opened this issue Jul 21, 2022 · 20 comments
Closed

Current status revision #8

vv-monsalve opened this issue Jul 21, 2022 · 20 comments

Comments

@vv-monsalve
Copy link
Contributor

vv-monsalve commented Jul 21, 2022

Current project status in the upstream repo https://github.com/benhoepner/National-Park at commit 9af5fb5. Bringing here Rosalie's comments from email + adding answers:

  1. Glyphsets
    Kernel is a glyphset for multiscripts font that don’t want to support much Latin. The actual minimal set we are requiring is Latin Core: https://github.com/googlefonts/glyphsets/blob/main/GF_glyphsets/Latin/txt/nice-names/GF_Latin_Core.txt.

Your font has a well developed glyph set, I would recommend to make sure you support:

  • Latin Core (missing three glyphs)
  • Latin Vietnamese (done)

I strongly recommend to also support Latin Plus.

Find attached a .plist file containing all GF Latin glyph sets. Add it in the same folder as your .glyphs files, and they will show in the « Filters » section when re-opening the file.

Globally, your figures are not set up correctly. I would recommend to read these tuto:

Your math signs are supposed to have the same width (same as the tabular figures preferably).

First of all, I would like to highlight the impressive progression of the project and the incredible work done :)

While I see the point about suggesting meeting the Latin Plus encoding, given the academic background of the project, I would suggest publishing it with the Latin Core now and leaving the Plus inclusion for a further upgrade.

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Jul 21, 2022

  1. Axes

I see 2 families:

  • Normal
  • Stencil

The « mind the gap » source, should be treated as additional masters in the stencil font to be able to create a variable axis out of it. If you don’t want to merge the files, you can use the « import font » parameter in font info, it is supposed to link the masters from the two sources to ensure outline compatibility whilst avoiding high number of masters in the source file. Seeing that you have a ExtraLight and Medium master, maybe you can try to interpolate the light master to see if you can get rid of it to reduce file size.

I also see only two families: Normal and Stencil. I'm under the impression the National-Park_MindtheGap.glyphs file was only an intermediate production file, but perhaps I'm getting it wrong. @aherstowski could you confirm the purpose of that source file.

Indeed, a Regular instance could be interpolated and then used as a master to reduce the number of masters to be maintained and the file size.

@vv-monsalve
Copy link
Contributor Author

  1. Onboarding

It looks like the « stencil » (or « gap ») axis are still at the very beginning (only four letters), so I wonder why the plan was to onboard the families in Q3.

This is more a question for Dave and Tobias;
Shall we onboard the Normal and Stencil fonts with only a weight axis, and then upgrade them with the « stencil » axis? Or shall we wait for the entire family to be ready (and register the custom axis) to onboard them?

From the above comment, I would say we could move forward with the Normal and Stencil fonts. However, let's wait for Andrea's confirmation about the third source file.

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Jul 21, 2022

4. Building + testing the fonts

I've noticed the build GHA has been failing and the fonts included on the repo seem to be directly exported from Glyphs app.

I received the same error when trying to build the fonts locally under a fresh venv with the command gftools builder config.yaml, but couldn't figure out what is causing it.

@simoncozens could you provide context on this?

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Jul 21, 2022

Summarizing the steps needed:

  • Confirmation of the purpose of the third source file to decide on the need for a custom axis for the project.
  • Please provide more detailed information in the README file. You could read about this file in our documentation.
  • Include a sample image in the README file
  • Either adjust or remove the trademark file.
  • Solving the problem of GHA build to build and test the sources and recover CI capabilities.
  • Add the five missing glyphs to comply with GF Latin Core
  • Improve the Dollar sign (using the bracket layer)
  • fsType should be set to zero ("Installable" in Glyphs app options)
  • Addition of copyright and license string in source files
  • Review vertical metrics to meet GF standards
  • Include build instructions in the README file

Please focus on the first items of the list, I could take care of the last four points once we have solved the build issue.

@simoncozens
Copy link
Contributor

I think the problem is that we temporarily added a line to requirements.txt because we were using glyphs3 before that was officially supported, and we did it in a weird way that is no longer supported. Remove the first line of requirements.txt and it should start working again.

@vv-monsalve
Copy link
Contributor Author

Remove the first line of requirements.txt and it should start working again.

I had that suspicion and created a venv and installed the requirements with that line removed, and got the same result. I'll run it again, in any case, to confirm and let you know. Thanks

This was referenced Apr 28, 2023
@vv-monsalve
Copy link
Contributor Author

The make build is reporting:

make: yq: Command not found
make: Nothing to be done for `build'.

I'm producing the fonts using the command gftools builder config.yaml under a venv for the time being.

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Jun 14, 2023

Notes after today's meeting with @aherstowski

  • We'll proceed with the Latin Core for first publishing and leave the Latin Plus for a later update.
  • A Regular master would be added to delete the Light and Medium and work only with three; extreme ones + Regular.
  • Families and stages:
    • The main standard version will be published first
    • The Stencil version onboarding will follow
    • Tentatively, a Stencil axis could be added later to the Stencil version to modify the stencil gap. Currently only tested on the MindtheGap source file.
  • Building process will be tested after merging Update template #11

@vv-monsalve
Copy link
Contributor Author

@simoncozens, the proof process is failing, reporting the error:

*** [Makefile:36: proof] Error 2. 
Error: Process completed with exit code 2.

At first, I wondered if it could be cause the fonts/ttf dir is not included in the repository. Although, if I understand it correctly, this is optional. What could be missing here?

@simoncozens
Copy link
Contributor

As a general tip, when you read an error message, it's always a good idea to look at everything from the last thing that worked onwards. The real problem is almost never in the last line of the log, because by that time everything has already gone wrong.

If we look a little bit further here, we can see:

gftools: error: unrecognized arguments: proof

And that makes me remember that we moved the proof command to diffenator2. You need to update your project template using make update-project-template (and then check in the changes).

@vv-monsalve
Copy link
Contributor Author

I reviewed the trace and saw indeed the line containing the word "proof", but some static fonts were listed thereafter, which is why I mistakenly assumed the issue was with the fonts since the diffenator2 also has a proof option. This, in turn, led me to overlook the gftools gen-html test line above after activating the virtual environment.

And that makes me remember that we moved the proof command to diffenator2. You need to update your project template using make update-project-template (and then check in the changes).

We'll do so. Thanks.

@vv-monsalve
Copy link
Contributor Author

After running make update-project-template the following error is reported:

npx update-template https://github.com/googlefonts/googlefonts-project-template/
make: npx: No such file or directory
make: *** [update-project-template] Error 1

Is it a dependency that should be installed separately (pip install npx)? Should it be added to the requirements file?

@simoncozens
Copy link
Contributor

npx is something that is part of node (the Javascript engine). You could use brew install node and that should do it. Otherwise I can just do the update for you and send you a PR.

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Aug 8, 2023

Otherwise I can just do the update for you and send you a PR.

I can brew-install it, thank you. However, when you say a PR, do you mean you would be adding it to the requirements file? Or what else would you be modifying? Would any other user of the template need to brew-install node?

I'm interested in ensuring this would be available for any user.

@simoncozens
Copy link
Contributor

You only need node when you are performing the update-project-template step. The update brings any changes to the base project files such as requirements.txt and Makefile which have been made in googlefonts-project-template into your project; for example, in this case we changed the Makefile in the googlefonts-project-template so that it calls diffenator2 instead of gftools gen-html, so you need to bring that change into your local project.

@vv-monsalve
Copy link
Contributor Author

The update brings any changes to the base project files such as requirements.txt and Makefile which have been made in googlefonts-project-template into your project;

This is clear as the goal of the update.

You only need node when you are performing the update-project-template step.

My question aims to clarify what is needed for any user who would like to update the template in the future. In this case, should we add the installation of node as a required step in the template's README file?

@simoncozens
Copy link
Contributor

Good idea, yes. Any maintainer using this upgrade step will need to have node installed. I'll add an issue to the template.

@vv-monsalve
Copy link
Contributor Author

vv-monsalve commented Aug 9, 2023

Great! Ty. Now, when brew installing it's requiring to change the ownership of some directories by running extra commands that require sudo log in. Should we warn the users about that possibility?

You should change the ownership of these directories to your user.
  sudo chown -R $(whoami) /usr/local/share/man/man8

And make sure that your user has write permission.
  chmod u+w /usr/local/share/man/man8

Edited:
By "Should we warn the users about that possibility?" I mean adding a line into the instructions like When installing Node, it may be necessary to change certain permissions that require sudo login. Please follow the instructions provided in terminal

@vv-monsalve
Copy link
Contributor Author

@aherstowski I've done a PR updating the template now

@vv-monsalve
Copy link
Contributor Author

@simoncozens I've added the above comment to the Template's issue. So I'm closing this issue here.

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

No branches or pull requests

2 participants