-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Webfonts: register and enqueue fonts by font-family #39559
Conversation
b979374
to
feb1de1
Compare
feb1de1
to
99521d4
Compare
@hellofromtonya this is ready for review and adapted to your comments. @aristath please take a look when possible 🙏 |
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 overall approach is looking good and makes sense to me.
I think this is best reviewed in tandem with #39593, as the first real test is if it helps us remove unused font-face declarations to avoid unnecessarily bloating the html.
b92fdda
to
b86cad6
Compare
f7a2b9d
to
9263fce
Compare
5dbbc48
to
10454fe
Compare
The API, although looking really good, is not ready for Core. So we must move it from wordpress-6.0 to experimental, and only add it there when it's ready to enter WordPress. Relocate files to /lib/experimental. Per the Gutenberg PHP > File structure documentation https://github.com/WordPress/gutenberg/tree/trunk/lib#file-structure.
Tests for methods that have a `_doing_it_wrong()` require an `expectedIncorrectUsage` annotation, proper usage of test fixture methods, and the usage of `__METHOD__` when invoking `_doing_it_wrong()`. This commit: - Fixes the test fixtures to use snake_case format and calling of the parent fixtures. - Refactors `get_font_slug()` tests into data providers (preferred in Core) and separates tests that will trigger `_doing_it_wrong()` to use the annotation. - Uses `__METHOD__` instead of `__FUNCTION__` in each `_doing_it_wrong()` instance.
The `WP_Webfonts::generate_styles()` should not be aware of how to reorganize the webfonts from grouped by font-family to grouped by provider. Why? This is a separate task from generating styles for the webfonts. This commit creates a private method called `get_webfonts_by_provider()` to encapsulate the know-how for this reorganizational work. It also includes a micro-optimization that eliminates an extra `foreach` loop and the building of an unnecessary `$webfonts` array of arrays.
f5f8339
to
90cc444
Compare
Rebased against latest |
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.
Scope of this PR - LGTM 👍
Other changes (such as tests, wiring theme.json enqueue, etc.) will be done in other PR(s).
return $variations; | ||
} | ||
|
||
class WP_Theme_JSON_Resolver_Gutenberg extends WP_Theme_JSON_Resolver_6_0 { |
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 file no longer seems to be loaded at all? I'd like to modify get_theme_data
but I'm not sure how now...
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.
Sorry @scruffian, what do you mean? It is listed in load.php
: https://github.com/WordPress/gutenberg/blob/trunk/lib/load.php#L111.
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.
Sorry, looks like I was stuck in a bad merge state...
Is this now the way fonts should be loaded into a block theme? Any official documentation on this? |
Hey @bradley2083, Please read #39332 (comment). |
@zaguiini Thanks for sharing that. So I'm still not 100% clear but sounds like we still need to use font-face declarations for the time being? |
Sorry, @bradley2083. We now have separated registration and enqueueing of fonts, just like we have with scripts and styles. #39988 will enqueue the fonts listed in After that, we'll work on #40363, so we can enqueue automatically the fonts picked in global styles. So no, you won't need to use font-face declarations. The Webfonts API should be sufficient. Right now, you can use it to register and enqueue webfonts through the API. When #39988 gets merged, you can use solely This problem will be addressed in #40363, so externally registered webfonts will be considered when enqueueing fonts to the front-end. |
Ok got it. Thanks @zaguiini I'll start to play around with using |
Yeah, right, @bradley2083. It's not going to be included in 6.0, so too early. But the methods in |
We noticed that with the new Gutenberg versions (from 13.0) the Global Styles for WC Blocks plugin doesn’t apply anymore. It seems that the problem is caused by this PR, in particular, this this change. I don't have a lot of context on this part of the code. Could you take a look? Thanks! |
What?
Part of #39332.
Why?
It's an attempt to make the webfonts API more flexible and save transferred data. We're now registering font families (individual font faces grouped by font-family) and enqueueing them as needed.
How?
We're creating a hash of the font-family attribute and grouping registered font faces to their respective font families;
We then introduce enqueueing and do not add registered font-families to the output by default -- they must be explicitly added through enqueueing. All fonts are enqueued when opening the editor so fonts picked when making changes show up correctly.
Webfonts should be enqueued after the
after_setup_theme
hook runs because that's where registration happens. So we should recommend hooking intoinit
orwp_loaded
to enqueue fonts since it's guaranteed to be registered at the time that these hooks are run.Testing a webfont loaded via
theme.json
:Using the Bennett theme as an example, we edit its
theme.json
file and replacefontFamilies
with the following:Then, we create a
functions.php
in thebennett
theme directory and add the following:Testing a webfont programmatically through PHP:
Next, to test the PHP registration using the
wp_register_webfont
function, infunctions.php
add the following:Refresh the page and Roboto should be there. You can try adding another font of the same family in italic (
font-style: italic
) and changing the src. It should load both fonts.When you remove
wp_enqueue_webfont
, Roboto should not be loaded anymore.