-
Notifications
You must be signed in to change notification settings - Fork 6
Add Google Fonts capability option #3
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
Add Google Fonts capability option #3
Conversation
Adds new font files
|
I can confirm that the files referenced are WOFF. The other assets committed to the branch still seem to be ttf though, and I'm not sure why. What I get locally is woff resources, so the change is right. I'm just not sure why I see some of the resources I do. I'm still running the scripts (it takes a while) so maybe I'll know more when it's done. |
|
The generation of the However running In the new version (17.6) it's slightly different: The following lines or variables have to change:
to return googleFonts.font_families
to ${families[i].font_family_settings.name}
to families[i].font_family_settings.fontFace.length
to const url = families[i].font_family_settings.fontFace[x]['src'];
With these changes, the font files get downloaded correctly as woff2 files. |
Co-Authored-By: Jessica Lyschik <[email protected]>
The API call fails if capability is empty
|
I'm seeing the following errors when generating the SVG previews: It looks like the library we're using, text-to-svg, doesn't support WOFF files. There's a similar discussion here: opentypejs/opentype.js#360. Maybe we should provide a list of TTF files for the previews to be generated from, and provide a list of WOFF files for the font assets themselves. Edit: I've spoken to @matiasbenedetto, and we discussed that we don't need to update the previews now and they're also only ever generated in this repo. So we can leave the preview file as it is for now, and handle the above error from text-to-svg in a follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid committing all the font files (WOFF or ttf) to the repo because we are not tracking changes on it, and they are making the repo big and difficult to handle (Github and vs. code PR previews not working) without a good reason. I think that previously, we were considering hosting the font assets ourselves for the font collection, and because of that, we persisted in the repo, but that's no longer true, so we can remove them.
Could you please remove the WOFF2 font assets from the PR so we can merge it?
I submitted a PR removing the existing TFF files from the repo: #5
I agree! Thanks for confirming, I'll remove the new files from this PR once #5 is merged. |
👍 Ready! |
|
I've updated this PR by removing font-assets. I also double-checked that the google-fonts.json is still up to date by running |
matiasbenedetto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds the Google Fonts capability option and sets it to
WOFF2. This means the font assets that are downloaded will beWOFF2instead ofTTF.This may be easier to review by looking at each commit, as the update to all the font files is making GH very slow 😅
Closes #1.
Testing Instructions
npm run filesWOFF2files