Google Fonts: enqueue only picked font families in Gutenberg #23810

zaguiini · 2022-04-05T23:24:09Z

Changes proposed in this Pull Request:

As WordPress/gutenberg#39559 got merged, we have more control over which font faces get to the front-end after registering.

This PR introduces code that introspects block usage and global styles configuration looking for Google fonts that have been registered.

Jetpack product discussion

N/A.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Setting up the environment:

Run trunk Gutenberg;
Map Gutenberg to Jetpack docker volume (doing this instead of a8c-wp-env because we had loads of trouble trying to get it working);
Run composer install inside projects/plugins/jetpack so the module gets linked with the plugin;
Enable the Google fonts module in Jetpack settings;

Enqueueing fonts used in blocks:

Open the site editor;
Include a block that allows typography customization;
Pick one of the registered Google fonts (e.g. Roboto);
Save the changes;
Open the site.

In the page source, check that only the picked font family shows up in the style#webfonts-inline-css element.

Enqueueing fonts used in global styles:

Open the site editor;
Open the global styles settings;
Pick one of the registered Google fonts (e.g. Roboto) in the typography section;
Save the changes;
Open the site.

In the page source, check that only the picked font families, both used in blocks above and the one that you just set in global styles, shows up in the style#webfonts-inline-css element.

github-actions · 2022-04-05T23:25:00Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: May 3, 2022.
Scheduled code freeze: April 25, 2022.

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

zaguiini · 2022-04-06T00:30:27Z

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

+	/**
+	 * Scan global styles for Google fonts.
+	 */
+	public function scan_global_styles_for_google_fonts() {


This function probably does not belong here. Right now, however, I'm struggling to find a better place to put it...

When it's time, we can probably move this to Gutenberg.

I wonder if this could be in the package for now, maybe in the Utils class?
https://github.com/Automattic/jetpack/tree/212b4d7bc0ab2be67b0ed7347f6496454e390052/projects/packages/google-fonts-provider/src

I actually created a brand new package. It didn't feel connected to the provider at all, and it could be used by other modules so hurray for reusability!

I wonder if this may be overkill. When used by third-parties, wouldn't that code pretty much always be used alongside the existing automattic/jetpack-google-fonts-provider package for now? Even if not fully related, maybe it could be a new class inside that package? We could always extract it into a new package if we start needing it elsewhere in the future, somewhere where the jetpack-google-fonts-provider package is not already required.

I wonder if this may be overkill.

I would agree; I think separating this out into its own package doesn't really provide much benefit. Realistically, you should always be introspecting fonts when using the provider with more than a few font faces (especially with how Google creates so many font face declarations for different character ranges). I'd hope that packaging the provider and introspection utilities together would encourage that.

If automattic/jetpack-google-fonts-provider as a package is too specific, I'd rather deprecate it and create a new package to more broadly encompass webfont functionality.

Additionally, having Global styles introspection and block introspection in separate places seems odd; I think these should be together.

Yep. Any reason why we created this jetpack-google-fonts-provider package in the first place? It looks weird to have this as separate parts.

And I do agree -- I think it's better if we collocated the provider and the introspector together within the Jetpack Google Fonts module. Thoughts?

Sorry if I wasn't clear, I don't mean stop using packages all together. My understanding is that Jetpack (the monorepo) is built around packages for functionality that's reusable across plugins. That way if Woo, for example, wants to use something that's in Jetpack, it doesn't have to copy the code into itself, or require install the entire Jetpack plugin be installed, it can just require the specific package it needs.

So for our use case, I think this separation makes sense:

Package: font provider, and any functionality needed for loading fonts from the provider. No hooks should be added by default, so it's up to package consumers to "turn on" any functionality from the package they want to use.

Module: Our WP.com specific list of curated Google Fonts, and calling the necessary functions from the package to load those fonts.

The main goal of the separation here being any other plugin could use the fonts package to load it's own set of Google Fonts, independent from the fonts module in the Jetpack plugin.

So, a few things:

The global styles introspection utils package is not adding any hook -- it's simply getting the global styles and looking for font families. However, that might be incomplete because only global styles introspection is not enough even for this use case as block settings are ignored. One thing I would like to have is an action called jetpack_content_introspection_webfont_found or something like that would get fired whenever a font family was found in the content;

The introspection package is not really related to the provider. Adding the provider does not add introspection, and vice-versa. One could use introspection without necessarily working with Google provider. The provider's job is to handle and emit the enqueued fonts, and that's it. It doesn't care about introspection.

The module could (and will) use both packages. Other modules might not, and only use the provider, or only use the introspection... I do think they should be decoupled as those are very useful things to expose.

Other modules might not, and only use the provider, or only use the introspection... I do think they should be decoupled as those are very useful things to expose.

Imagining myself as a developer with 0 knowledge of the fonts work we've done, I'd find it unintuitive to need to import two separate packages to enqueue webfonts in a performant way. That is to say, I wouldn't think to look for an introspector while setting up fonts support.

I agree with Jeremy + Grant -- I suspect there'd be more clarity in colocating both introspection and provider logic in the same package for now. I understand that it would be strange to place everything inside the jetpack-google-fonts-provider, but like Grant mentioned earlier, I'd be on board with creating a new, more appropriately named package that contains everything.

Either way, I see that we've decided to punt this decision for later -- just my 2 cents.

jeherve

I left a few comments below, let me know what you think!

Also, could you add a changelog entry to your PR, using jetpack changelog add plugins/jetpack?

Thank you!

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

projects/plugins/jetpack/modules/google-fonts/jetpack-google-fonts.php

jeherve · 2022-04-06T07:22:45Z

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

+	/**
+	 * Scan global styles for Google fonts.
+	 */
+	public function scan_global_styles_for_google_fonts() {


I wonder if this could be in the package for now, maybe in the Utils class?
https://github.com/Automattic/jetpack/tree/212b4d7bc0ab2be67b0ed7347f6496454e390052/projects/packages/google-fonts-provider/src

projects/plugins/jetpack/modules/google-fonts/jetpack-google-fonts.php

matticbot · 2022-04-07T00:59:27Z

Caution: This PR has changes that must be merged to WordPress.com
Hello zaguiini! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D78348-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

zaguiini · 2022-04-07T15:02:01Z

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

+		/**
+		 * We are already enqueueing all registered fonts by default when loading the block editor,
+		 * so we only need to scan for webfonts when browsing as a guest.
+		 */
+		if ( ! is_admin() ) {
+			add_action( 'wp_loaded', array( $this, 'enqueue_global_styles_google_fonts' ) );
+			add_filter( 'pre_render_block', array( $this, 'scan_block_for_google_fonts' ), 10, 2 );
+		}


Need to keep it like that until Gutenberg lazy loads font-face declarations as they're picked in the block editor.

jeyip · 2022-04-11T19:38:48Z

Not entirely certain if it's just me, but I keep running into critical errors when running this locally with the instructions provided.

PHP Fatal error:  Uncaught Error: Call to undefined function Automattic\Jetpack\Fonts\introspect_global_styles_webfonts() in /usr/local/src/jetpack-monorepo/projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php:144
Stack trace:
#0 /var/www/html/wp-includes/class-wp-hook.php(307): Jetpack_Google_Fonts->enqueue_global_styles_google_fonts()
#1 /var/www/html/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters()
#2 /var/www/html/wp-includes/plugin.php(474): WP_Hook->do_action()
#3 /var/www/html/wp-settings.php(609): do_action()
#4 /var/www/html/wp-config.php(86): require_once('/var/www/html/w...')
#5 /var/www/html/wp-load.php(50): require_once('/var/www/html/w...')
#6 /var/www/html/wp-blog-header.php(13): require_once('/var/www/html/w...')
#7 /var/www/html/index.php(17): require('/var/www/html/w...')
#8 {main}

What I'm doing:

Pull latest changes for Gutenberg
Install Gutenberg npm modules
Build trunk Gutenberg
Map Gutenberg to Jetpack docker volume ( doing this instead of a8c-wp-env because I had loads of trouble trying to get it working )
Check out this jetpack branch
Install jetpack package dependencies pnpm jetpack install -all
Spin up jetpack dev environment pnpm jetpack docker up
Navigate to http://localhost

What I'm seeing:

creativecoder · 2022-04-13T15:03:57Z

Not entirely certain if it's just me, but I keep running into critical errors when running this locally with the instructions provided.

I was getting the same error, but I think the latest changes (that add "automattic/jetpack-global-styles-webfonts-introspector": "0.1.x-dev", to the Jetpack plugin's composer.json) fix the problem.

Note you'll need to run jetpack install -r and/or jetpack install --all after pulling.

creativecoder

The functionality here is working as expected. Running with current Gutenberg trunk:

Without the change, fonts appear in Font Family dropdowns in the editor, but no fonts are loaded on the front-end.
With the change, fonts from Global styles or block settings in the editor are loaded on the front-end.

We just need to figure out the right balance of package vs. module functionality (see my other comments) and I think this will be ready to go.

creativecoder · 2022-04-14T23:03:11Z

@zaguiini Here's a draft PR to illustrate what I was trying to say in the comments: #23958

Hold off on creating a separate package for now, it adds admin overhead, and we can always do this later. If Gutenberg ends up adding this functionality, we'll avoid the hassle of having to deprecate the new package.
Keep a clean separation between the fonts introspection and fonts module. The module only needs to add the appropriate hooks and the font introspection classes do everything else.
Keep things modular so it's easy to add another introspector, when font settings are added to other places in Gutenberg.
(Add some tests to better document how things work.)

Let me know what you think!

zaguiini · 2022-04-14T23:46:35Z

Thanks, @creativecoder!

You made very good points, I agree with all of them.

In addition, there are some things I'd like to observe...

All found font faces are enqueued, regardless if we were the ones that registered them

The implementation in my PR tries not to enqueue every single font that gets detected by introspection. We're keeping track of the fonts we registered, then, when a font is found, we're checking if it was registered by us before enqueueing.

This tightens things a bit, so only the subset of fonts we registered get enqueued... I wonder how could we implement that intermediary state without the use of a class in a reliable way? One possibility is to get the list of registered fonts from the API (wp_webfonts()->get_registered_webfonts()), grab the faces by their slug, and look for the family name of the first font face in the Google Fonts list... But this looks more like a workaround than a solution.

That's why I transitioned into a class, and that's the purpose of the registered_google_fonts property in the Jetpack_Google_Fonts class: to have this 1-1 mapping from the slug to the name. Then, we have this maybe_enqueue_* method that looks in this map before enqueuing the font family.

Could we achieve something similar without classes? Maybe using global variables or a singleton class but I'm not sure how common this pattern is in Jetpack...

Introspection classes in the same plugin as the Google fonts provider

It looks really off to have the introspection classes inside the Google fonts provider plugin. Those are different things that could work without each other... But I cannot think of a better place to put it without creating a new plugin. If there's absolutely no better place to put it (how about putting it in the module?), I'm fine with leaving it where it is.

Right now they do have some degree of relationship because as you read in my first point, we want only to enqueue the fonts that we registered.

However, I see them as more general. It would be really nice if we could abstract the global styles and block introspection and apply an action or filter whenever a registered font is found. Exposing that as a plugin, I can see a lot of external plugins using it to do the same thing within their own plugins.

What I'm trying to say is that it's not really tied to Google -- it has a lot of potential reusability by third parties. Unless, of course, we don't want them using it.

Migrating to Gutenberg

Not specific to our implementations, but how are going to migrate from Jetpack to Gutenberg when it's time? Is there any guideline on how to deal with these situations? Should we add a guard that would not hook up our own filters in case Gutenberg already did so? Curious about how it'll work...

Thinking about the migration to Gutenberg. Introspection is going to happen there at some point (see WordPress/gutenberg#40363 and WordPress/gutenberg#40386), so the introspection code included here is temporary.

All things considered... Does it make sense to include it on a package instead of simply adding it to the module? I believe it's easier to remove things from a module than to deprecate a plugin, right? If that's the case, why bother adding it to a package?

Sorry for the wall of text, but those are my concerns 😄

jeyip · 2022-04-15T10:21:46Z

Could we achieve something similar without classes? Maybe using global variables or a singleton class but I'm not sure how common this pattern is in Jetpack...

Out of curiosity, is there a reason we'd prefer not to use a class to implement this logic?

jeyip · 2022-04-15T16:26:52Z

...ckages/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php

+	}
+
+	/**
+	 * Introspect global styles for webfonts.


Could we add an @return inline comment here for consistency?

jeyip · 2022-04-15T18:10:29Z

...ckages/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php

+				return $matches['slug'];
+			}
+
+			return $font_family;


Could you describe what this line would return and under what circumstances this condition would be triggered? I'm guessing one of the presets we evaluate ( blocks, HTML, or global typography ) return a normal slug.

Also completely unrelated to this PR, but it seems so odd to have so many different ways of representing the font family slug 🙃

Also completely unrelated to this PR, but it seems so odd to have so many different ways of representing the font family slug 🙃

Right! That's because you can specify a CSS variable to link it inside theme.json. You can also pick a font in the editor, and then it would become the second format var:preset|font-family|slug, which is the one that's registered in the global styles CPT.

But then, you can also solely specify the slug, and that's why we're returning... If none of the formats match, just return what's in there and we'll try our luck. It might be a system font, might be just the slug... Who knows.

...ckages/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php

jeyip · 2022-04-15T18:23:25Z

...ckages/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php

+
+		$found_webfonts = array();
+
+		// Look for fonts in block presets...


Non-blocking: How would we feel about providing a little more context for those that might be unfamiliar with global styles?

// Global styles can specify typography for individual blocks, // so look for fonts in block presets... // Global styles can specify typography for elements like headings, paragraphs, etc., // so look for fonts in HTML element presets...

That's more explicit, for sure! I'm curious if this is necessary since it kind of goes without saying that global styles can specify the typography since we're looking for them in the code?

I'm curious if this is necessary since it kind of goes without saying that global styles can specify the typography since we're looking for them in the code?

Sure -- for those examples, I see what you're saying. My intention is to ultimately provide clarity for others by adding context about business use cases, especially since this code will live in Jetpack for the time being ( where folks might not be as familiar with the global styles feature ).

Does that make sense? Maybe we can think about different, more meaningful wording.

Gotcha. Sure, we can think of something.

jeyip · 2022-04-15T19:20:25Z

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

+
+			if ( empty( $font_family_slugs ) ) {
+				// Fonts were not registered.
+				continue;


Could this lead to errors being silently hidden?

Not really -- we're always emitting errors in the front-end so they'll be noticed.

Hmmm...I might be missing something but here's the flow of logic I have in my head ( I might be mistaken about this! )

We'd attempt to register webfonts

register_webfont method would return false ( link )

wp_register_webfonts would fail and return an empty array of font_family_slugs

We wouldn't enqueue any fonts because registered_google_fonts would be empty

The frontend would silently fallback to some system or default font

When would errors be emitted on the frontend?

The wp_register_webfonts method runs for every font family declared in the list, so there'd be multiple runs and not a single one.

And yes, that's mostly right. One thing, though, is that the user/developer would be notified after register_webfont method would return false on the front-end, so they'd have a clue on how to fix what went wrong.

register_webfont method would return false on the front-end, so they'd have a clue on how to fix what went wrong

Ah I see -- that's what I was missing. When you have time, would you mind explaining how returning false would notify the user / dev in the frontend? The only way I can see this working is with error logging.

It's not for the front-end. Return false is more about conditional action upon registration failure for example. I do agree, though, that we're lacking descriptive return values on failure when dealing with the back-end (i.e. false can mean multiple things). But that's not something to address in this PR.

It's not for the front-end.

Oh you had mentioned the front-end a few times in previous comments so there might have been a miscommunication ( link )

But that's not something to address in this PR.

Agreed

Oh, my bad.

What I tried to say was that a notice is displayed on the front end, so the user can take action.

jeyip

The code generally looks good to me. @creativecoder made a number of great points though

Regarding modularity and having global styles introspection play well with other introspection logic...from what I can tell, what's written here would work given those considerations.
Tests! Totally agree.

+1 from me when my last few comments are answered and tests are written.

zaguiini · 2022-04-15T21:21:21Z

projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php

+	 * @param string $font_family_slug The block content.
+	 */
+	private function maybe_enqueue_font_family( $font_family_slug ) {
+		if ( ! isset( $this->registered_google_fonts[ $font_family_slug ] ) ) {


We need to check if the font is registered before trying to enqueue it:

Suggested change

if ( ! isset( $this->registered_google_fonts[ $font_family_slug ] ) ) {

if ( ! wp_webfonts()->is_font_family_registered( $font_family_slug ) ) {

The is_font_family_registered function will be exposed after WordPress/gutenberg#39988 gets merged. For now, what we can use is:

Suggested change

if ( ! isset( $this->registered_google_fonts[ $font_family_slug ] ) ) {

if ( ! isset( wp_webfonts()->get_registered_webfonts()[ $font_family_slug ] ) ) {

zaguiini · 2022-04-15T21:32:31Z

After meeting with @jeyip and @creativecoder, we came up with a few observations:

We might not need a class for the Jetpack Google fonts module

The reason for having a class was to share the state between hook calls: the one that registers webfonts, and the introspection callbacks (e.g. pre_render_block).

This shared state existed so we could be a little more cautious regarding which fonts get enqueued. We wanted to make sure that only the fonts we registered were enqueued, and that's a safety measure. However, that's too cautious.

See, right now, Gutenberg is in this weird state where externally registered webfonts picked in the editor do not have any effect on the front end. Removing the guard clause would allow any picked font in the editor to show up in the front end.

This is the worst-case scenario, and one that we actually want to have as it would temporarily fix Gutenberg's behavior (see WordPress/gutenberg#40363 and WordPress/gutenberg#40386).

A note regarding the implementation is this comment: https://github.com/Automattic/jetpack/pull/23810/files#r851520186. We still need to check if the font is registered before enqueueing it since we're not sure it was registered before now that we'll get rid of the intermediary state. Not checking and trying to enqueue a webfont that was not registered will add a notice on the front end.

We might need additional information on where to put the introspection classes

@jeherve could really help us here as we're uncertain about where to put them. It still feels unnatural to add the introspection classes to the Google provider package. Ideally, we should create a new package to expose a separate thing since one can use the introspection class without necessarily using the Google font provider, and vice-versa.

But, how much of a hassle is it to create, deploy and maintain packages in WPCOM? Should we? How can we balance the trade-off between having a well-organized codebase and the burden of maintaining a new package? The answers are beyond our reach right now, but @Automattic/jetpack-crew might have them all.

Migrating to Gutenberg

How are going to migrate introspection from Jetpack to Gutenberg when it's time? Is there any guideline on how to deal with these situations? Should we add a guard to the module that would not hook up our own filters in case Gutenberg already did so? Curious about how it'll work...

Introspection is going to happen there at some point (see WordPress/gutenberg#40363 and WordPress/gutenberg#40386), so the introspection code included here is temporary. Please take that into consideration when answering the questions on the "We might need additional information on where to put the introspection classes" paragraph.

Shepherding the PR to `master`

I'm going OOO for the next three weeks starting on Monday, so @Automattic/cylon will take care of it. Please reach them out as necessary, but I believe the discussion will be centralized here.

Addison-Stavlo · 2022-04-18T14:48:57Z

How are going to migrate introspection from Jetpack to Gutenberg when it's time?

We may not ever need to. We may even remove the introspection from Jetpack when it makes sense.

My takeaways from this long slack thread (p1649889597371109-slack-CHN6J22MP) with @mtias were:

Fonts in theme.json and global-styles (saved theme.json) are to be enqueued on the front-end. (p1649957191543249/1649889597.371109-slack-CHN6J22MP)
Programmatically registered webfonts are not intended to be available anywhere except global styles, the fact that they appear in individual block settings font pickers is a current bug in that system that will need to be resolved (p1649957314019679/1649889597.371109-slack-CHN6J22MP). In the end individual block settings will only list theme supplied fonts and not programmatically registered fonts.

When this is the case, there is no need for all the introspection since fonts available to individual blocks are already enqueued from being part of theme.json and any fonts selected by global styles would be enqueued from there.

From my understanding, the introspection is now only a temporary solution to allow Google fonts on dotcom sooner. This serves to band-aid the gaps in enqueueing that should not be present in the end state of the Gutenberg webfonts API. It fills the gap of not enqueueing global styles selections (which should be resolved soon?). This also allows us to fill the gap in the current/temporary state of registered webfonts showing up in individual block settings - this part seems reasonable to me as once Gutenberg removes these webfonts from this section any previous selections on dotcom should be back-wards compatible due to this introspection code.

Addison-Stavlo · 2022-04-21T13:22:02Z

the fact that they appear in individual block settings font pickers is a current bug in that system that will need to be resolved

It seems I may be mistaken about this point. Im still not certain of Gutenberg's plans but this part may remain in place meaning our introspection here would remain valuable for all the individual block-settings.

creativecoder · 2022-04-21T14:53:42Z

To move this forward, I'd like to get some guidance from @Automattic/ground-control on whether to create a new package or not.

You can see how I've attempted to cleanly divide the Google Fonts provider from the font enqueuing ("introspection") code in this draft PR: #23958.

On the one hand, the enqueuing code is potentially useful as it's own package for use with other font providers, inside or outside Jetpack. On the other, the WP Webfonts API is still maturing and not really stable yet, so significant changes my be needed, or that code might become irrelevant, so it may be worth waiting.

Also, I know creating a separate package incurs some overhead, particularly on wpcom.

Do you have a preference on keeping the code for the Introspectors classes (from #23958) within the Google Fonts provider, or creating a separate package for it, at this time? We'd like to wrap this up and get it shipped asap.

Thanks!

jeherve · 2022-04-22T15:56:50Z

@jeherve could really help us here as we're uncertain about where to put them.

Sorry, I was AFK for the past few days, only getting back to this now.

All in all, and given that this is all building atop something that's not part of WordPress Core, thus not widely adopted yet, I would vote towards keeping it simple so we can move and iterate quickly. In practice, I think that means less packages. Even though a new package may prove useful to third-parties in the future, I don't think it will be useful to many people right now. Since there are still many moving pieces in the new API, I don't think anyone would reasonably start building their own solution on top of 2 separate Jetpack packages that themselves rely on a rapidly changing Gutenberg-only feature.

In short, I would:

Not worry too much about what we do today; we can always iterate later as things get more stable in Core, and as the questions to Gutenberg are answered.
Not create a new package today.
Keep things in the Jetpack plugin itself and in the existing package.
- Global_Styles_Webfonts_Introspector could be a new class in the package.
- projects/plugins/jetpack/modules/google-fonts.php could require and initialize projects/plugins/jetpack/modules/google-fonts/class-jetpack-google-fonts.php. We can require more files in projects/plugins/jetpack/modules/google-fonts.php if it becomes necessary.

What do you think?

creativecoder · 2022-04-22T18:24:19Z

Sorry, I was AFK for the past few days, only getting back to this now.

@jeherve No problem! Thanks for prioritizing a reply here so soon after getting back.

What do you think?

That makes sense to me. I'll close this in favor of #23958 and move forward there.

zaguiini requested review from creativecoder and a team April 5, 2022 23:24

zaguiini self-assigned this Apr 5, 2022

github-actions bot added the [Feature] Google Fonts label Apr 5, 2022

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Apr 5, 2022

zaguiini added the [Status] In Progress label Apr 5, 2022

zaguiini changed the title ~~Introspect Google fonts used by Gutenberg~~ Introspect Google fonts used in Gutenberg Apr 6, 2022

zaguiini changed the title ~~Introspect Google fonts used in Gutenberg~~ Enqueue only the used Google fonts in Gutenberg Apr 6, 2022

zaguiini changed the title ~~Enqueue only the used Google fonts in Gutenberg~~ Enqueue only the Google fonts used in Gutenberg Apr 6, 2022

zaguiini force-pushed the update/instrospect-google-fonts-instead-of-enqueueing-them-all branch 2 times, most recently from e7674df to e0d1aad Compare April 6, 2022 00:20

zaguiini changed the title ~~Enqueue only the Google fonts used in Gutenberg~~ Google Fonts: enqueue only picked font families in Gutenberg Apr 6, 2022

zaguiini commented Apr 6, 2022

View reviewed changes

jeherve reviewed Apr 6, 2022

View reviewed changes

zaguiini force-pushed the update/instrospect-google-fonts-instead-of-enqueueing-them-all branch from e0d1aad to 86b6972 Compare April 6, 2022 23:55

github-actions bot added the [Package] Global Styles Webfonts Introspector label Apr 6, 2022

zaguiini force-pushed the update/instrospect-google-fonts-instead-of-enqueueing-them-all branch 2 times, most recently from 29ee55d to 7d5316b Compare April 7, 2022 00:19

zaguiini requested a review from jeherve April 7, 2022 00:19

zaguiini force-pushed the update/instrospect-google-fonts-instead-of-enqueueing-them-all branch from 7d5316b to ce6f7bc Compare April 7, 2022 00:59

matticbot added the Touches WP.com Files label Apr 7, 2022

zaguiini commented Apr 7, 2022

View reviewed changes

zaguiini added 6 commits April 11, 2022 19:05

Instropect blocks and enqueue used Google fonts

279c275

Instropect global styles and enqueue used Google fonts

8b31d4a

Get font family slug through wp_register_webfonts output

2896b93

Extract global styles webfonts introspector to its own package

9bab79d

Use webfonts introspector when looking for fonts used in global styles

b660425

Add CHANGELOG entry

b9c92a4

zaguiini force-pushed the update/instrospect-google-fonts-instead-of-enqueueing-them-all branch from 9670f69 to b9c92a4 Compare April 12, 2022 00:09

zaguiini mentioned this pull request Apr 13, 2022

Webfonts: enqueue fonts listed in theme.json WordPress/gutenberg#39988

Closed

creativecoder reviewed Apr 13, 2022

View reviewed changes

zaguiini mentioned this pull request Apr 14, 2022

Google Fonts: Enqueue fonts used in block and Global Styles #23958

Merged

jeyip reviewed Apr 15, 2022

View reviewed changes

...ckages/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php Show resolved Hide resolved

jeyip reviewed Apr 15, 2022

View reviewed changes

zaguiini commented Apr 15, 2022

View reviewed changes

creativecoder closed this Apr 22, 2022

github-actions bot removed [Status] In Progress [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 22, 2022


		$found_webfonts = array();

		// Look for fonts in block presets...

	if ( ! isset( $this->registered_google_fonts[ $font_family_slug ] ) ) {
	if ( ! wp_webfonts()->is_font_family_registered( $font_family_slug ) ) {

	if ( ! isset( $this->registered_google_fonts[ $font_family_slug ] ) ) {
	if ( ! isset( wp_webfonts()->get_registered_webfonts()[ $font_family_slug ] ) ) {

Google Fonts: enqueue only picked font families in Gutenberg #23810

Google Fonts: enqueue only picked font families in Gutenberg #23810

Conversation

zaguiini commented Apr 5, 2022 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeherve Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaguiini Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Apr 7, 2022

Choose a reason for hiding this comment

jeyip commented Apr 11, 2022 • edited Loading

creativecoder commented Apr 13, 2022

creativecoder left a comment

Choose a reason for hiding this comment

creativecoder commented Apr 14, 2022

zaguiini commented Apr 14, 2022 • edited Loading

All found font faces are enqueued, regardless if we were the ones that registered them

Introspection classes in the same plugin as the Google fonts provider

Migrating to Gutenberg

jeyip commented Apr 15, 2022

Choose a reason for hiding this comment

jeyip Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaguiini commented Apr 15, 2022 • edited Loading

We might not need a class for the Jetpack Google fonts module

We might need additional information on where to put the introspection classes

Migrating to Gutenberg

Shepherding the PR to master

Addison-Stavlo commented Apr 18, 2022 • edited Loading

Addison-Stavlo commented Apr 21, 2022

creativecoder commented Apr 21, 2022

jeherve commented Apr 22, 2022

creativecoder commented Apr 22, 2022

zaguiini commented Apr 5, 2022 •

edited

Loading

github-actions bot commented Apr 5, 2022 •

edited

Loading

jeherve Apr 7, 2022 •

edited

Loading

zaguiini Apr 13, 2022 •

edited

Loading

jeyip Apr 15, 2022 •

edited

Loading

jeyip commented Apr 11, 2022 •

edited

Loading

zaguiini commented Apr 14, 2022 •

edited

Loading

jeyip Apr 15, 2022 •

edited

Loading

jeyip Apr 15, 2022 •

edited

Loading

jeyip Apr 15, 2022 •

edited

Loading

jeyip Apr 15, 2022 •

edited

Loading

zaguiini commented Apr 15, 2022 •

edited

Loading

Shepherding the PR to `master`

Addison-Stavlo commented Apr 18, 2022 •

edited

Loading