Closed stephenhutchings closed 8 months ago
Thanks @stephenhutchings. I briefly looked at all the source files and everything looks good. I tagged this for review for onboarding to Google Fonts in Q4. We will review this submission in more detail soon.
Hello @stephenhutchings. This project has been accepted for onboarding to Google Fonts and we are moving forward with the onboarding process this week with Reddit Sans as a high priority project.
I left some issues in the project's issue tracker. These are things that have come up in the QA process and need to be fixed before onboarding the fonts. If for some reason these changes don't make sense in the upstream project's repository, Google Fonts can maintain a separate fork that meets all our QA requirements, but Ideally the project at github.com/reddit/redditsans would be as close to the version on Google Fonts as possible.
The main, most important issues that need to be addressed before onboarding are:
fsType
must be set to 0
Also, do you have plans to release this as a variable font at some point? If so, would it make sense to combine Reddit Sans and Reddit Sans Condensed into one variable font with a weight and width axis?
I'm working on an updated version of the source files that makes the vertical metrics consistent across the family now, and can make a PR to your upstream repository with that file if you like, but I understand you will probably want to review the vertical metrics issue yourself and might have some opinions on how it is handled. Is there a good reason why the vertical metrics are now consistent across the family? Let me know.
Thanks!
Thanks @eliheuer. Appreciate the detailed comments.
It won't be a problem addressing all of the issues you have raised in the main project, so there is no need to maintain a separate fork. I should be able to resolve all of those issues this week. With respect to inconsistent vertical metrics, that seems to be an oversight rather than intentional.
A variable font is a possibility with separate width and weight axes. However, there are currently no italics in the condensed width so it would be limited to the roman fonts. Nothing is planned at the moment, but I'll discuss this further with the Reddit team. If we do go down that route, what effect does that have on the Google Fonts side of things?
I've released a new version (v1.012) that addresses the bulk of issues that you've highlighted.
There are two remaining issues related to copyright. If these are non-critical, Reddit's preference is to leave the copyright as is.
We merged a first version to see what it looks like in sandbox and speed up the onboarding process of the font, but it is not complying with our requirements so we would be waiting for an update :)
We would need:
Thanks for the update @RosaWagner.
If @eliheuer is happy to set up the gftools build step that would be much appreciated. I can make the other changes to the repo structure next week.
Hey @RosaWagner @eliheuer ! Following up here. Stephen, our font designer made the necessary updates to Reddit Sans (v1.012) outlined here.
Question regarding this copyright convo. We'd like to keep the copyright attribution as "Reddit, Inc.". We have a CLA with Google in place. I wanted to check in and make sure this suffices here? I'll note that we will be updating the copyright URL to link out to the Reddit Sans repo, per your team's suggestion.
Other than these items, is there anything else needed from us to move forward? TY!
Checking in here! Is there anything else needed from our team here? Thank you! @RosaWagner @eliheuer
@tr000y Hello, I will look into the copyright issue. The main problem is that the fonts need to be built with the Google Fonts build system described here: https://googlefonts.github.io/gf-guide/build.html
But the fonts currently use .glyphsproject
files which are not supported.
I will try to get a fork working with the Google Fonts build system as an example if possible.
@eliheuer I made a bit of progress in the build step on the gf-onboarding branch. This required some changes to the source files, and I need to do a review to ensure the built fonts don't vary significantly from the current production fonts.
I'm not sure it's possible to get the build working without changing the source files, but if you have any insight here that would be great.
@eliheuer, as told in the meetings: we don't need to support the .glyphproject
files, everything can be set up to export the default version of Reddit Sans
(Roman + Italic), Reddit Sans Condensed
and Reddit Mono
using the builder (from the source files). .glyphsproject
files are not sources, they are just an alternative way to set up instances in the GlyphsApp environment. Please ignore the subsets A B C etc for now, they are not meant to be onboarded right away, the first step and priority is for the 3 families in their default form.
@stephenhutchings
.yaml
file (one for each families).I don't think @eliheuer handled a case of building multiple families in one repo with intermediate layers etc before, so I allowed myself to step in to show a way to handle that case. I arranged the sources in a fork to be able to build with gftools builder, but I didn't go into details for the QA. Also, I set up an "ideal" scenario which might differ from the "desired" output (explanations below).
link: https://github.com/RosaWagner/redditsans/tree/gf-onboarding
What I did in the sources
Light
master at 32
and an Light
instance at 64
, therefore I renamed the master Light
to ExtraLight
and the master Bold
to Black
. .yaml
file for each families.fonts
directory.What to do about: the range of the weight axis is larger than the proposed set of instances. There are different possibilities:
copyright We can grant exception. @davelab6 just needs to confirm.
The blocker for this project The intermediate layers in glyphsLib is having an issue: https://github.com/googlefonts/glyphsLib/issues/954 As long as this issue is open we can't provide the exact same design as before, which is a bummer.
Here again there are several possibilities:
random stuff I noted but didn't fix
.cap
instead of .case
and the export for them is disabled. You need to export them to have a proper mark_to_base
and mark_to_mark
features for the uppercases.@stephenhutchings, once you've chose you preferred scenarios, @eliheuer can take over.
@RosaWagner @eliheuer Thanks so much for outlining all of this! Our preference for 1. range of weight axis and 2. the glyphsLib issue is option two in both scenarios. For both issues, is this something GF would handle, or will we need to update on our end?
Thanks @tr000y, to make it clear you choose:
We can provide the bash script that exports the range you prefer, but you would have to add the intermediate layers everywhere (it might require a bit of scripting so it doesn’t take ages). Do you want to do that from the sources I already set up?
I made the bash script and created a PR to @eliheuer's fork so he can take over (https://github.com/eliheuer/redditsans/pull/1). varLib.instancer
is provoking a suspicious FAIL with ots sanitizer
that we need to investigate. The team based in the US is in thanksgiving holidays so I don't know how much time it's going to take.
If you feel the emergency to fix the rest of the todo listed in the PR, I can also PR the current state of the branch to your branch.
Edit: issue with ots sanitizer filled in https://github.com/fonttools/fonttools/issues/3350
@RosaWagner please file an issue in fonttools about the instancer and will take a look, thanks
I've updated our branch to include @RosaWagner's changes. It also has intermediate layers added to component glyphs, plus a few fixes to other issues that were raised in eliheuer/redditsans#1.
Hi @RosaWagner @eliheuer, do you need anything else from our side to move forward? Thank you!
Hi @tr000y and @stephenhutchings, Eli is supposed to QA and onboard :)
As a curiosity I checked how Words on Windows would handle the naming of the VF Reddit Condensed
, and indeed it is a bit confused. Windows uses the STAT table to display font names and font styles, and I have no idea why the tool is adding a width axis in the STAT table; it results in styles called Reddit Sans Condensed Condensed
on Words. This is something that needs investigation (cc @simoncozens @m4rc1e). Since Condensed is in the family name, it shouldn't be found anywhere else for the description of styles names to avoid confusing Windows.
Changing "Condensed" into "Narrow" everywhere seems to stop triggering the addition of a wdth axis, so it can be a workaround. But it would be better if the tool stops adding a wdth axis in the STAT table (I'll open an issue in gftools)
Otherwise the ots fail is fixed, so if you are happy with the builds, @eliheuer could start with onboarding Reddit Sans and Reddit Mono.
Thank you for the update @RosaWagner! Yes, we're happy with the builds and would like to move forward with Reddit Sans and Reddit Mono.
Re: Condensed, I will check in with my team regarding the workaround you mentioned (renaming to Narrow). But please also let us know if you figure anything out on your end.
Thanks @RosaWagner and @tr000y. I will try to make a new pull request onboarding Sans and Mono.
Hi @tr000y and @stephenhutchings, Eli is supposed to QA and onboard :)
As a curiosity I checked how Words on Windows would handle the naming of the VF
Reddit Condensed
, and indeed it is a bit confused. Windows uses the STAT table to display font names and font styles, and I have no idea why the tool is adding a width axis in the STAT table; it results in styles calledReddit Sans Condensed Condensed
on Words. This is something that needs investigation (cc @simoncozens @m4rc1e). Since Condensed is in the family name, it shouldn't be found anywhere else for the description of styles names to avoid confusing Windows.Changing "Condensed" into "Narrow" everywhere seems to stop triggering the addition of a wdth axis, so it can be a workaround. But it would be better if the tool stops adding a wdth axis in the STAT table (I'll open an issue in gftools)
Otherwise the ots fail is fixed, so if you are happy with the builds, @eliheuer could start with onboarding Reddit Sans and Reddit Mono.
@RosaWagner Who do we need to involve at this stage to address this?
Marc is Mister STAT Table. :-)
cc @m4rc1e, but I think he is on vacation until the end of the year, I can do my best to look into it in his absence.
It can be workaround with patching a stat table without width axis with gen-stat at the end of the process using the bash script already there
I'm currently looking into this issue. Will report back or fix.
The STAT table for the Condensed family is being generated by the googlefonts axisregistry. I need to do some investigating to see whether it is complaint with the MS spec.
For the time being, we could just declare a STAT table in the sanscondensed.yaml yaml file? I'll happily do this.
I have no idea why the tool is adding a width axis in the STAT table
The Condensed Axis Record is being added because it found a Condensed token in the name table. It most likely used nameID 1 or 16. Perhaps we should only be discovering name record token using NameID 2 or 17.
@m4rc1e except if I missed something, when I tested that, it still added the width axis. I explained more in details the problem in this issue: https://github.com/googlefonts/gftools/issues/801. But I tested a lot of stuff recently with different projects, so I might have confused something, does it work when you do it?
Defining a stat table in the condensed yaml file works for me. I don't get a Condesed axis. PR submitted, https://github.com/reddit/redditsans/pull/13
Hello @tr000y and @stephenhutchings! I'm Emma, I'm the new onboarder assigned to that project! I tested the fonts from here: https://github.com/reddit/redditsans/commit/011df2f10c4af552b7eedd49eb19da725b3132af
Here you can find my review for Reddit Sans (I'll start the Reddit Mono today). Feel free to ask if you have any questions! Cheers!
/ Global comment The Glyphset is slightly different in italic (around 50 glyphs missing). It’s better to have the same supported langages both in upright and italic. Here, it concerns stylistics sets and symbols. For stylistics sets, there aren’t supported by the API, so it’s ok to not have them in italics. For symbols, it would be nice :)
Version: In the version exported I checked, it’s the 1.012, should be bumped to 1.014 (to avoid any confusion with that PR: https://github.com/google/fonts/pull/6850 and with the version on the main branch of your repo -which is not the last one if I well understood-)
/ Fontbakery report remarks
Fails: 🔥 Copyright: we are agree for this exception 🔥 Check variable font instances: With Rosalie, you agreed to have a 300-800 range instead of 200-900.
What can be improved to remove WARNS:
(To check in every fonts)
Are there caret positions declared for every ligature? -> add anchors in each ligatures (caret ones)
Remove soft hyphen (no longer in the GF Glyphset)
Interpolation issues, only in the upright: northEastArrow.case southEastArrow.case southWestArrow.case northWestArrow.case leftRightArrow.case
Check math signs have the same width -> It’s better, but if not it’s ok to leave them like this.
Italic: same remarks + weird behaviour with the Caron (looks a bit far in the regular master)
https://github.com/google/fonts/assets/64773544/21540bd7-87e2-4657-a8ac-0c00857e573f
fontbakery version: 0.10.9
💔 ERROR | ☠ FATAL | 🔥 FAIL | ⚠ WARN | 💤 SKIP | ℹ INFO | 🍞 PASS | 🔎 DEBUG |
---|---|---|---|---|---|---|---|
0 | 0 | 4 | 25 | 207 | 15 | 243 | 0 |
0% | 0% | 1% | 5% | 42% | 3% | 49% | 0% |
Note: The following loglevels were omitted in this report:
For Reddit Mono:
-> Same remarks as for Reddit Sans for license url and soft hyphen -> Please consider that WARN: ⚠ WARN: Checking correctness of monospaced metadata.
Small outlines issues:
circumflex doesn't look well aligned
https://github.com/google/fonts/assets/64773544/e0e5dc92-c367-480a-9252-25f5392e8da8
Same in the O
The y's dot is a bit too much on the right:
Same issue with Lcaron
fontbakery version: 0.10.9
💔 ERROR | ☠ FATAL | 🔥 FAIL | ⚠ WARN | 💤 SKIP | ℹ INFO | 🍞 PASS | 🔎 DEBUG |
---|---|---|---|---|---|---|---|
0 | 0 | 2 | 10 | 110 | 8 | 126 | 0 |
0% | 0% | 1% | 4% | 43% | 3% | 49% | 0% |
Note: The following loglevels were omitted in this report:
Hello @stephenhutchings, What is the status here? Do you have any questions about this feedback?
Cheers!
Hi @emmamarichal. I am looking at these issues, although I have limited time due to other work. Hopefully I'll make some headway in the next few days.
@emmamarichal I believe the latest fonts on the gf-onboarding branch address most issues you raised above, except for one.
The position of the ogonek component on "ę" seems to be correct in the source files and the non-variable font exports. I am not sure why the variable fonts use a different position. Any help here would be appreciated.
@stephenhutchings Yes sorry, for the ogonek, I think I looked at the .ttf and it's normal to have this kind of issue in a font editor. But all looks great otherwise. Thank you!
You can merge this branch to the main one so I can start the onboarding process. For the condensed version, not sure to understand: do you want to wait the italic version in order to make a variable font later? (with two axis: weight and width) Or do you want to onboard it now, following the discussion above, with Marc's changes?
@emmamarichal It's the end of the day for me here but I'll look at getting this merged into master this week.
Let's move forward with onboarding the condensed without italics. Marc's changes to the sanscondensed.yaml
file have been merged onto that branch, so they will come across at the same time.
Ok, since I didn't know what was the decision about it, I didn't review the condensed version yet. Let me do it today, I'll add a comment here with my review :)
Weird caron position:
https://github.com/google/fonts/assets/64773544/4343f6d1-7697-4b6a-847a-306f7e448b9d
In uni022B, the macron seems very closed to the dieresis, compared to uni022A:
Position issue:
https://github.com/google/fonts/assets/64773544/bbf9eab0-edc7-4565-b898-fc4291a93b6c
Thanks @emmamarichal I will look at getting these fixed as soon as possible.
@emmamarichal These issues are now fixed and merged onto the main branch, with release tag v1.014
.
Hey @emmamarichal, following up here. Is anything else needed on our end to move forward? Ty!
Ah yes sorry, I was quite busy last week! Will take a look this week and see if I can do a PR to the GF repo :)
Font Project Git Repo URL: https://github.com/reddit/redditsans
Super short description of the Font Family: Reddit Sans is a humanist sans-serif designed for Reddit. Reddit Sans is complemented by Reddit Sans Condensed and Reddit Mono, which are also contained in the same repository. Fonts can be previewed on the microsite.
Requirements:
Google Fonts will publish only fonts that match its requirements. Please familiarize yourself with the complete documentation in the Google Fonts Guide (GF-Guide) and ensure your font project complies with them before submitting the font family. You can also use the Google Fonts Project Template, which will help you create a repository that follows the needed structure and includes build requirements.
By filling this issue, you can confirm the project meets the requirements (by ticking the cases or putting x between the square brackets in text mode):
Image:
Attach here a pic or a screenshot of the font. One image is enough, it can be a few letters, to give a quick overview.