Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Fonts API] Refactor theme.json global functions into the WP_Fonts_Resolver #50914

Closed
wants to merge 3 commits into from
Closed

[Fonts API] Refactor theme.json global functions into the WP_Fonts_Resolver #50914

wants to merge 3 commits into from

Conversation

hellofromtonya
Copy link
Contributor

Part of #50250.

What?

Refactors the 2 theme.json global functions (in lib/experimental/fonts-api/register-fonts-from-theme.json.php) by placing them into the existing WP_Fonts _Resolver and then splitting the tasks into separate internal static methods.

  • gutenberg_register_fonts_from_theme_json() -> WP_Fonts_Resolver::register_fonts_from_theme_json()
  • gutenberg_add_registered_fonts_to_theme_json() -> WP_Fonts_Resolver::add_registered_fonts_to_theme_json()

Why?

Copied from the enhancement issue:

These functions are large and doing multiple tasks, as such are difficult to debug and understand.

Their role is to handle the interactions between the global theme.json settings (which includes the user selected typography from Site Editor's > Global Styles > Typography UI) and the Fonts API. But that role is not clearly defined with the file and function naming.

To improve code quality, readability, and prepare these functionality for future and Core, this file and its functions need a redesign / refactoring. It's a better fit for a Core-only static class wrapper pattern (i.e. a class with static methods).

How?

  • Use [Fonts API] Refactor Theme JSON global functions into WP_Fonts_Resolver #50811 as a starting point - cherry-pick and smash the commits into 1 commit to reference its PR.
  • Move the code into WP_Fonts_Resolver and remove the added class. Why? These methods are resolving between theme JSON data and the Fonts API, i.e. they are resolver functionality.
  • Relocate the tests for each of the methods.

Testing Instructions

Manual Testing

How these functions work and what they do did not change. This means the functionality should work exactly as it did for, i.e. scope of this PR should be a pure refactoring. Manual testing is focused on ensuring no changes happened to the functionality, i.e. works the same as in trunk.

Setting up your local test site:

  1. Make sure Gutenberg is activated: Plugins > Gutenberg.
  2. Install and activate this test plugin.
  3. Activate the TT3, if it is not already.

Testing the theme's fonts get automatically registered and enqueued into the Fonts API:

  1. Open the Site Editor > Styles > Typography UI.
  2. Select Heading.
  3. In the Fonts selector, verify the theme's fonts are present for selection.
  4. Go to the front-end.
  5. In your browser's dev tools in the Inspector (HTML) tab, search for wp-fonts-local. Expectation: It should be in the <head> and contain the @font-face styles for the theme's fonts.

Testing plugin fonts are added to the Theme JSON data layer:

  1. Open the Site Editor > Styles > Typography UI.
  2. Select Heading.
  3. Select a Google Font for all headings:
    • For Fonts: select Playfair Display.
    • For Appearance: select Bold Italic.
  4. Select the "Save" button twice, which saves your user selections.
  5. Go to the front-end and refresh the page.
  6. In your browser's dev tools in the Inspector (HTML) tab:
    • Search for wp-fonts-jetpack-google-fonts. Expectation: The <style> element should be in the <head> and the CSS in it should only contain @font-face styles for Playfair Display.
    • Search for wp-fonts-local. Expectation: It should be in the <head> and contain the @font-face styles for the theme's fonts.

Automated Testing

Run the following:

npm run test:unit:php -- --group fontsapi

All tests should pass.

@anton-vlasenko @hellofromtonya
Start with PR 50811.
d7298b8 
Props anton-vlasenko.

Cherry picked the following commits from PR 50811:

 Rename register-fonts-from-theme-json.php to class-wp-fonts-theme-json-handler.php.

 Guard the class.

* Move gutenberg_register_fonts_from_theme_json() and gutenberg_add_registered_fonts_to_theme_json() to static methods.

* Rename WP_Fonts_Theme_Json_Handler::gutenberg_register_fonts_from_theme_json to WP_Fonts_Theme_Json_Handler::register_fonts_from_theme_json.

* Rename WP_Fonts_Theme_Json_Handler::gutenberg_add_registered_fonts_to_theme_json to WP_Fonts_Theme_Json_Handler::add_registered_fonts_to_theme_json.

* Move part of the code to WP_Fonts_Theme_Json_Handler::parse_font_families.

* Fix PHPDoc block.

* Move anonymous function into a static method.

* Fix PHPDoc block.

* Move methods within the file.

* Rename to ::get_font_families.

* Rename ::get_families to ::get_font_families.

* Fix CS.

* Fix @Covers

* Update PHPDoc blocks.

* Rename files.

* Rename files.

* Update PHPDoc blocks.
@hellofromtonya hellofromtonya added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Feature] Fonts API labels May 24, 2023
@hellofromtonya hellofromtonya self-assigned this May 24, 2023
@hellofromtonya hellofromtonya force-pushed the try/fonts-api/refactor-resolver-global-functions branch 2 times, most recently from c0aba18 to c996030 Compare May 24, 2023 16:49
@hellofromtonya hellofromtonya force-pushed the try/fonts-api/refactor-resolver-global-functions branch 4 times, most recently from dfa461f to 2c8a4ce Compare May 25, 2023 15:22
@WordPress WordPress deleted a comment from github-actions bot May 25, 2023
@hellofromtonya
Further refactoring.
f9a56e4 
* Splits more tasks into separate methods.

* Defines the "registered in Fonts API origin" as a const in WP_Fonts.
Why? To centralize the defined origin, rather than dispersing it in multiple places.

* Renames from `add_registered_fonts_to_theme_json()` to `add_missing_fonts_to_theme_json()`.
Why? Its job to add "missing" fonts, which could be registered or enqueued depending upon
the need. For example, block level typography needs the enqueued fonts, whereas global
level needs registered.

* Housekeeping:
Chandes "webfont" to "font".
Adds @SInCE to each method.
@hellofromtonya hellofromtonya force-pushed the try/fonts-api/refactor-resolver-global-functions branch from 2c8a4ce to f9a56e4 Compare May 25, 2023 17:46
@hellofromtonya hellofromtonya mentioned this pull request May 25, 2023
Merged
9 tasks
@WordPress WordPress deleted a comment from github-actions bot May 25, 2023
@github-actions
Copy link

Flaky tests detected in f9a56e4.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5082987781
📝 Reported issues:

@hellofromtonya
Copy link
Contributor Author

Closing this experiment in favor of #50811. The work done in this PR has been combined into PR 50811.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey will be requested when the pull request is marked ready for review spacedmonkey is a code owner

Assignees

@hellofromtonya hellofromtonya

Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hellofromtonya @anton-vlasenko