-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: enqueue fonts listed in theme.json #39988
Conversation
edaa21d
to
f5f8339
Compare
55397b0
to
5c963e9
Compare
f5f8339
to
90cc444
Compare
2fc8d5a
to
6eed9b7
Compare
adcc6ce
to
12d41f8
Compare
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.
Where should I put utility functions? Should I keep them as functions prefixed by gutenberg_
or put them in the WP_Webfonts
class?
If the former applies: should we move WP_Webfonts::get_font_slug
elsewhere? Should we create a file so the utils are concentrated somewhere, or should we keep them collocated where they're defined/used?
Good questions, I don't have a perfect answer here. Generally speaking, if a function gets called outside of an internal class context, we prefer to have raw global functions with a In the case of something like |
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 is some really good work! ✨
I think we should look at a different approach for gutenberg_add_registered_webfonts_to_theme_json
here, though. As I am understanding it, this is adding any registered webfont to theme.json
so that those fonts will be available in the Editor, yes?
If so, I'd like to explore a more sensible approach where that isn't happening all the time. We certainly don't need to be patching all of those font families in for the frontend, where this will pay a performance penalty that we don't need or want. We should find out where the Editor is pulling in fonts and only run it under that condition.
How about we don't run this function when requesting the front-end, @mattwiebe? We could check |
That is a fine place to start and probably good enough for this pass. We can always iterate on it down the road if issues crop up. |
12d41f8
to
10e9964
Compare
Did some moving @mattwiebe. Also, we're not running the add registered webfonts to |
Test ReportEnv:
Setup:
Testing Steps
Testing Results:For steps 1-5: ❌
For steps 6-7: ❌
ResultsTT2's webfonts are not being enqueued and the font-faces not generated. ❌ |
Ran the same tests against the current version in |
Adds a guard clause to check if the attribute exists in each webfont before the comparison happens. If no, it bails out, returning false, as there's nothing to compare. Some girl scouting: * Renames the function's properties to be more readable. * Adds `array` type declaration as only arrays are accepted. * A wee bit of alignment for consistency in Core.
The font-family in each `fontFaces` may be different than its parent. For example, the parent may have a fallback defined such as "fontFamily": "\"Source Serif Pro\", serif",`.
The errors noted in my test report are fixed by the last 2 commits ✅ bd4259e uses each font-face's dc14844 resolves the notice for missing index:
|
1. Added @SInCE annotations. 2. Removed empty line between last @param and @return in DocBlock. 3. Renamed variables that were using PHP reserved key words, such as `$array`. Required to avoid PHP 8.x deprecation notices and PHP 9 fatal errors. 4. Used `empty()` instead of `count()` for readability and consistency. 5. Moved `_wp_array_keys_to_kebab_case()` to group the array key transformation functions. 6. A wee bit of girl scouting in the DocBlocks for readability and consistency.
This reverts commit bea59de.
@hellofromtonya I went ahead and did some cleanup/refactor in the utility functions. I don't think we should be introducing new functions like that - unless absolutely necessary.
|
bb569ae
to
e2fe01c
Compare
Let's remove this PR from the 6.0 Project Board as the alternative #40472 was implemented for WP 6.0 |
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
1 similar comment
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
The Fonts API is deprecated, replaced by Font Face to work with the Font Library. Enqueuing is no longer needed. Thus this PR can be closed without merging. |
Part of #39332.
What?
We want to enqueue only webfonts listed in
theme.json
.And we also want to register webfonts if needed by passing a
provider
key in a font face declaration, along with any additional properties required by the specified provider.Why?
Save data. Each font face declaration might add up to 2kb to every page load. Allow-listing webfonts through a centralized theme file -- like
theme.json
-- makes it possible for the theme developer to pick only a subset of the registered fonts.How
The logic runs in three steps:
Testing
Externally registered font families not listed in
theme.json
They should not be enqueued.
Enqueue an externally registered font family
In
functions.php
:In
theme.json[settings][typography][fontFamilies]
:Only
Roboto
should be enqueued.Enqueue only one of all externally registered font faces
In
functions.php
:In
theme.json[settings][typography][fontFamilies]
:Only
Roboto:900:bold
should be enqueued.Register (and enqueue) a collection of font faces to the same provider
In
theme.json[settings][typography][fontFamilies]
:Both faces of
Roboto
should be enqueued by the same provider.Register (and enqueue) a collection of font faces through different providers
In
theme.json[settings][typography][fontFamilies]
:Both faces of
Roboto
should be enqueued through different providers.