Google Fonts: update the method used to preconnect Fonts source domain. #23322
Conversation
Follow-up to #23045 Let's favor using a Core implementation with a Core filter instead of manually adding to the head.
jeherve
added
[Package] Google Fonts Provider
github-actions
bot
added
the
[Plugin] Jetpack
|
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:
|
creativecoder
left a comment
creativecoder
left a comment
There was a problem hiding this comment.
This works great. TIL about wp_resource_hints!
creativecoder
left a comment
creativecoder
left a comment
There was a problem hiding this comment.
Spoke too soon 🙂. This works great for adding the preconnect link, but we still need a check before adding the filter.
|
|
||
| // Run on an early priority to print out the preconnect link tag near the start of the page source. | ||
| add_action( 'wp_head', '\Automattic\Jetpack\Fonts\Google_Fonts_Provider::preconnect_font_source', 0 ); | ||
| add_filter( 'wp_resource_hints', '\Automattic\Jetpack\Fonts\Google_Fonts_Provider::font_source_resource_hint', 10, 2 ); |
There was a problem hiding this comment.
If Gutenberg is disabled, I'm seeing PHP Warning: Invalid argument supplied for foreach() in /Users/Grant/Automattic/Sites/wordpress-develop/src/wp-includes/general-template.php on line 3346, as the font_source_resource_hint method is not available to hook into.
There was a problem hiding this comment.
Ah right, because the parent class of Google_Fonts_Provider isn't available. What do you think about creating a new class that won't depend on WP_Webfonts_Provider. Would that be overkill? The approach you took in the other PR works, but I'm thinking doing things with a new class, and doing the WP_Webfonts_Provider check in there, would simplify things for folks having to implement that filtering.
What do you think?
WP_Webfonts_Provider may not always be available, so we cannot call a method from a child class of `WP_Webfonts_Provider` from a WP filter. Let's introduce a new class instead, that doesn't depend on anything, and do the class check inside there. That should simplify things a bit for the folks having to implement that filtering.
…te/google-fonts-preconnect
kraftbj
added
[Status] Ready to Merge
github-actions
bot
removed
the
[Status] Ready to Merge
|
Sorry for the delay in me getting back to this... thanks for taking care of it! |
Follow-up from #23045
Fixes #23306
Changes proposed in this Pull Request:
Instead of manually outputting a link to the
head, let's rely on a Core filter,wp_resource_hints, to take care of that for us.Let's take that opportunity to ensure we only do that when the Webfonts API is supported on the site.
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: