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

Unblock publication of Georama #2650

Closed
davelab6 opened this issue Aug 26, 2020 · 10 comments · Fixed by #3358
Closed

Unblock publication of Georama #2650

davelab6 opened this issue Aug 26, 2020 · 10 comments · Fixed by #3358

Comments

@davelab6
Copy link
Member

https://github.com/google/fonts/pull/2578/files was merged, but is blocked from publication:

georama's OS_2 weight class (400) doesn't match with default outline location (100).

This will also effect METADATA

@davelab6 davelab6 assigned m4rc1e and RosaWagner and unassigned m4rc1e Aug 26, 2020
@RosaWagner
Copy link
Contributor

RosaWagner commented Aug 31, 2020

Noted.
Questions:
A. I never unblocked a publication: (1.) it consist in merging the modifications in the same PR, or (2) I create a new branch since the PR is merged anyway?
B. The fonts have to be (1) reexported with correct usWeightClass, or (2) do you prefer a hotfix?

@m4rc1e
Copy link
Collaborator

m4rc1e commented Sep 1, 2020

georama's OS_2 weight class (400) doesn't match with default outline location (100).

We've had this issue before fonttools/fontbakery#2683. The solution was to set the METADATA.pb weigthClass value to 400. We've done exactly the same thing for Georama so I cannot see why this is being flagged. We also update FB and gftools to do this automatically.

@thlinard
Copy link
Contributor

thlinard commented Sep 1, 2020

Although the version number is the same, the files of #2578 are behind the upstream repo: they don't have the fix for productiontype/Georama#4, while the upstream VFs have it.

@davelab6
Copy link
Member Author

davelab6 commented Sep 5, 2020

weight: 400
has it set to 400

@davelab6
Copy link
Member Author

davelab6 commented Sep 5, 2020

@RosaWagner a. Unblocking publication just means that this seemed ready to go live but got stuck at he last stage, so it's a priority to do the last fixes and get it finished. 1. The pr was merged, so more can't be added there any more. More can be added to the branch that was merged and a new pr made. So you can do as you say in 2 also.

For b, I prefer re-export, but I recommend to always be in touch with upstream before doing any fixing yourself as they might volunteer to fix it themselves

@RosaWagner
Copy link
Contributor

Thanks @davelab6. I have 2 questions

  1. What about @m4rc1e comment:

We've had this issue before fonttools/fontbakery#2683. The solution was to set the METADATA.pb weigthClass value to 400. We've done exactly the same thing for Georama so I cannot see why this is being flagged. We also update FB and gftools to do this automatically.

  1. The variable font contains width instances. So same as Newsreader: is this font special, or I should remove some instances and get in touch with @productiontype about it ?

@RosaWagner
Copy link
Contributor

The binaries has also too long PS names (over 31 characters), so I would suggest to reexport it all anyway with contracted family names… Do we have a standard for that by the way ? I have seen this post fonttools/fontbakery#2179 but it doesn't really set up a standard.

@productiontype
Copy link

We usually do use short names in our own workflow, but they are known to cause stylelinking issues in some apps. After discussions on NewsReader, it was decided to implement long names in both projects. See commit productiontype/Newsreader@24d0eee See discussion short vs long on FB: fonttools/fontbakery#2179

@m4rc1e
Copy link
Collaborator

m4rc1e commented Oct 8, 2020

weight: 400

has it set to 400

I still don't see how this is an issue.

We currently serve Raleway which has an OS/2 usWeightClass value of 100 (default origin is Thin). While in its metadata, it has 400. Here's an FB issue which discusses the relationship between the METADATA.pb and the OS/2 VF value, fonttools/fontbakery#2683

@RosaWagner RosaWagner removed their assignment Oct 13, 2020
@m4rc1e
Copy link
Collaborator

m4rc1e commented Oct 19, 2020

Sorry, I now see the issue.

The font has a default origin of 100, yet the usWeightClass is 400. the usWeightClass should be 100.

@RosaWagner RosaWagner linked a pull request May 21, 2021 that will close this issue
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 a pull request may close this issue.

5 participants