-
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
[Fonts API] Separate BC Layer #50077
Conversation
7fda0cc
to
9409177
Compare
9409177
to
5678e2d
Compare
* WP_Fonts now extends from WP_Dependencies. * WP_Webfonts uses wp_fonts() rather than directly interacting with WP_Dependencies properties. * WP_Webfonts::register_webfont() uses wp_register_fonts(). Also removed an unused private method.
a912f1a
to
b8da418
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.
I've done an initial code review.
Please see my questions below.
Thank you.
lib/experimental/fonts-api/bc-layer/class-gutenberg-fonts-api-bc-layer.php
Show resolved
Hide resolved
lib/experimental/fonts-api/bc-layer/class-gutenberg-fonts-api-bc-layer.php
Outdated
Show resolved
Hide resolved
lib/experimental/fonts-api/bc-layer/class-gutenberg-fonts-api-bc-layer.php
Show resolved
Hide resolved
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
lib/experimental/fonts-api/bc-layer/class-gutenberg-fonts-api-bc-layer.php
Outdated
Show resolved
Hide resolved
Oh sorry about that @anton-vlasenko. I've updated the testing setup instructions. Yes, set its Appearance to |
Hmm interesting. Let me run through the testing setup and instructions to double check my findings, i.e. just in case there's something wrong there. I'll report back shortly. |
I retested. It worked on my local setup as I fixed the bug in the Jetpack Google Fonts package (see below). Env:
@anton-vlasenko I think the paragraph text is right, i.e. using Poppins. But looks like the Playfair Display is not enqueued. There's a bug in the Jetpack Google Fonts package Before this PR, calling This commit should make the Google Fonts work again. Retesting now. Can you test too please? |
Thanks, @hellofromtonya.
I agree with you about the paragraph text, my apologies for the confusion. dfad50f fixed the issue for me, and now the headings are displayed with the correct font. |
Retested with the latest and it works! Rather than using my fixed version of the Google Fonts package, I cloned @ironprogrammer's plugin. Using the updated PR with the fix, it works as expected. Excellent catch @anton-vlasenko 👏 The issue was: It required a polyfill in the BC Layer to ensure sites keeping working once this PR get merged and ships. The bug in Jetpack needs to be fixed in its repo. But to maintain BC (and avoid sites from breaking until it's fixed in that package), I've added a polyfill: Note: Once automatic enqueuing of user-selected fonts is merged into the API, this area of code in the Google Fonts package is no longer needed. This is why I haven't yet opened an issue in Jetpack to fix this bug. |
Thank you explaining that to me, @hellofromtonya. |
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.
I've tested this PR and it worked as described (after applying dfad50f).
The code also looks good to me.
Therefore, I approve this PR.
Fixes #50073
What?
The goal is to separate the BC Layer from the Core intended Fonts API code.
With this PR, the BC Layer remains fully functional by invoking the Fonts API's functionality but without making the Fonts API dependent upon the BC Layer.
Why?
To prepare the API for Core introduction. Why? The BC Layer is Gutenberg specific and will not be merged into Core. Currently this layer is intermingled within the API's code.
How?
WP_Webfonts
from theWP_Fonts
inheritance.wp_webfonts()
back to instantiatingWP_Webfonts()
but injectswp_fonts()
into it for reuse.wp_register_fonts()
towp_register_webfonts()
.deprecations
folder tobc-layer
for readability and better alignment to its purpose.Originally,
WP_Webfonts
was in the inheritance chain to ensure its public functionality kept working for extenders who may be invoking them. For example, Jetpack's Google Fonts Provider package < v0.5.0 useswp_webfonts()
andWP_Webfonts::get_registered_webfonts()
.Testing Instructions
Automated Tests
Manually Testing
The key to testing is to compare before and after this PR with emphasis on:
@font-face
styles in the iframed editor and front-end HTML?Set up:
'Poppins'
font in theJETPACK_GOOGLE_FONTS_LIST
constant. Why? This is the font used in the BC Layer test plugin.Testing Instructions:
Playfair Display
font and set Appearance toBold Italic
. Expected behavior: headings typography should change to look likePoppins
font and set the Appearance toRegular
. Expected behavior: the paragraph typography should change to look like.HTML testing:
The following
style
tags should exist:The BC Layer style should be like this:
style
elements. Are all 3 there?style
elements. Are all 3 there? (They should be in the<iframe>
.)style
elements. Are all 3 there?Screenshots
In the Site Editor
On the front-end
In a Post