-
Notifications
You must be signed in to change notification settings - Fork 805
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
Google Fonts: Integrate with new font library #33203
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
9b2459c
to
efbdf69
Compare
At first shake I notice: Enabling and dis-abling this module causes the fonts introduced by this module to stick around. I didn't test a lot of scenarios though, just tinkered with it for a bit. Ultimately this seems like a lot of logic in place for something the Font Library should be doing; I'm concerned that if this is they way Jetpack would augment the Font Library then the pipes to work with the Font Library aren't quite right yet. I would rather try to do this more simply while the Font Library is relatively malleable. Ideally: The module could provide a list of fonts to make available with a |
Yes, so I'd prefer to keep the Google Fonts invisible on the Font Library modal in the current iteration to avoid people controlling the fonts from the Jetpack. I'm not sure whether the Google Fonts module has to remove the added fonts from the Global Styles when disabling this module (and is it possible?).
Yep, I just noticed we have the collection of the JSON file but my concern is the Maybe keeping the font assets in the Jetpack repo makes more sense but I'm unsure whether Jetpack allows us to add those font assets 🤔 |
33ffa44
to
7910c08
Compare
require_once __DIR__ . '/current/load-google-fonts.php'; | ||
} elseif ( class_exists( 'WP_Fonts' ) || class_exists( 'WP_Webfonts' ) ) { | ||
// WordPress 6.3 compat. | ||
require_once __DIR__ . '/wordpress-6.3/load-google-fonts.php'; |
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.
Calling this "WordPress 6.3 compatible" isn't exactly correct.
It's more accurate to say that it's "Gutenberg Fonts API compatible" (since the API it leverages is available only in Gutenberg, that library MAY be available in other versions of WordPress, etc)
* @package automattic/jetpack | ||
*/ | ||
|
||
if ( class_exists( 'WP_Font_Library' ) && class_exists( 'WP_Font_Face' ) && class_exists( 'WP_Font_Collection' ) ) { |
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.
The Font Library won't be available in WordPress 6.4 since it's currently only available in Gutenberg. It's planned to be available in WordPress 6.5. However this technique IS available in situations other than WordPress 6.4 (or 6.5).
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.
Are there any changes I have to make here? I made a change to only check the WP_Font_Face
class as I assume we still rely on it to generate and print font styles. WDYT?
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.
The class isn't actually used or depended on at all as the logic exists now, WP_Font_Collection WAS used but now that collection data is just loaded manually and the class isn't needed. The data provided in that file that is loaded is everything needed for the fonts to work; no extra logic needed. That allows the logic to be font library/api agnostic. Doing it like this means that it ONLY works in Gutenberg, wereas it COULD work WITHOUT Gutenberg. Considering that it HASN'T worked without Gutenberg in the past I don't know if that matters though.
So I guess the answer is "If you want this to work WITHOUT gutenberg it CAN you'll just need to change that if() logic a bit." "If you don't care for this to work without Gutenberg then it's fine and dandy as it is"
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.
Okay. I'll leave it as it is since it hasn't worked without Gutenberg in the past. I'd prefer to make it work without Gutenberg after WordPress 6.5 or above which supports the WP_Font_Face
class.
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.
This tests well for me, and the changes make sense. I left a few nit-picks below, but nothing blocking (maybe the links, since we really try to stay away from private links in public repos in general?).
projects/plugins/jetpack/modules/google-fonts/wordpress-6.3/constants.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/google-fonts/current/load-google-fonts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/google-fonts/current/load-google-fonts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/google-fonts/current/load-google-fonts.php
Show resolved
Hide resolved
Oh yes! We have to 👍 |
Related to Automattic/wp-calypso#81875
Proposed changes:
generate_google_fonts
CLI tool on wpcom.wp_theme_json_data_default
hook so we can display Google Fonts in the dropdown of the Font Family.However, the default fonts are invisible on the block settings as the block settings pick font families from one of the origins. But I assume it's acceptable for now since the user will only see the installed fonts there if they install a font. As a result, I think it makes to inject them into the default now(The issue is resolved 🎉)Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
/wp-admin/admin.php?page=jetpack_modules
to activate the “Google Fonts (Beta)” module