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

[WIP] Change Webfonts API architecture to use Core's Dependencies API #42946

Closed
wants to merge 16 commits into from
Closed

[WIP] Change Webfonts API architecture to use Core's Dependencies API #42946

wants to merge 16 commits into from

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Aug 3, 2022 •
edited
Loading

Closes #41481.
See PR #40556 for the Proof of Concept (PoC).

THIS PR IS NOT READY. It's in development.

What?

Changes the Webfonts API's architecture to use WordPress Core's Dependencies API.

TODO

  • Merge the PoC
  • Get register working with theme.json
  • Get enqueue working with theme.json
  • Ensure all registrations functions go through validation first
  • Get remove working
  • Get dequeue working with theme.json
  • Test with plugins
  • Test with other providers
  • Update all of the unit tests
  • Do performance benchmarking

Why?

... details will be coming when the PR is ready ....

How?

... details will be coming when the PR is ready ....

Testing Instructions

Screenshots or screencast

Dependencies

This PR requires the following PRs to be merged into trunk and then trunk merged into here:

@hellofromtonya hellofromtonya self-assigned this Aug 3, 2022
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Aug 3, 2022 •
edited
Loading

Benchmark: Average Processing Time

  • Tasks: register and enqueue (same code as at the bottom of gutenberg_register_webfonts_from_theme_json()).
  • Web fonts data: Uses the data from gutenberg_register_webfonts_from_theme_json().
  • Number of cycles: 100,000
  • How did I do it? See the instructions and code here

Ran on 3 Aug 2022

Results:

Architecture Average milliseconds (ms) for 100k cycles
Current 0.6217
New (this PR) 0.5496

Ran on 17 Aug 2002

Commits used:

Results:

Architecture Average milliseconds (ms) for 100k cycles
Current 0.0207
New (this PR) 0.0267

@hellofromtonya
Register and enqueue working.
1425f32 
Using the POC, makes the register and enqueue work.

Reworks the `do_items()` and `do_item()` to process each
provider's fonts, rather than font-by-font.

Adds a `print_styles()` method to the base Provider.
Why? To give each provider control its HTML tag and styles.
@hellofromtonya hellofromtonya force-pushed the try/webfonts-architecture-extend-wp_dependencies branch from a745dcf to 1425f32 Compare August 4, 2022 22:17
hellofromtonya
hellofromtonya commented Aug 4, 2022
View reviewed changes
lib/experimental/webfonts.php

foreach ( $webfonts as $webfont ) {
$slug = wp_register_webfont( $webfont );
foreach ( $webfonts as $font_family => $variations ) {
Copy link
Contributor Author

@hellofromtonya hellofromtonya Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PoC changes the structure of the $webfonts array as now the variations must be a multi-dimensional array keyed by its font-family.

Previous structure:

array(
	array(
		'provider'     => 'local',
		'font-family'  => 'Source Serif Pro',
		'font-style'   => 'normal',
		'font-weight'  => '200 900',
		'font-stretch' => 'normal',
		'src'          => 'https://example.com/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2',
		'font-display' => 'fallback',
	),
	array(
		'provider'     => 'local',
		'font-family'  => 'Source Serif Pro',
		'font-style'   => 'italic',
		'font-weight'  => '200 900',
		'font-stretch' => 'normal',
		'src'          => 'https://example.com/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2',
		'font-display' => 'fallback',
	),
)

New structure:

array(
	'source-serif-pro' => array(
		array(
			'provider'     => 'local',
			'font-style'   => 'normal',
			'font-weight'  => '200 900',
			'font-stretch' => 'normal',
			'src'          => 'https://example.com/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2',
			'font-display' => 'fallback',
		),
		array(
			'provider'     => 'local',
			'font-style'   => 'italic',
			'font-weight'  => '200 900',
			'font-stretch' => 'normal',
			'src'          => 'https://example.com/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2',
			'font-display' => 'fallback',
		),
	),
)

I can see the benefits of grouping variations to their font family before passing it to this function. That said, it will have an impact on plugin authors who are already using the API.

With that in mind, @aristath @desrosj what do you both think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the older format in theme.json keeps working, then changing this will be fine 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could look for an associative array, and if associative keys are not present trigger a _doing_it_wrong(), and then just move the font-family value to the key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could look for an associative array, and if associative keys are not present trigger a _doing_it_wrong(), and then just move the font-family value to the key.

I have a mock up of this and was considering it for a first phase migration. Not sure if this would be need though once merged into Core. But would help while still in experimental phase in Gutenberg.

What do you think @desrosj @aristath ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at a dozen or so block themes in the directory and I couldn't find any that were using the current implementation.

However, my stance is usually that any time we change an API that is publicly released in a version of WordPress, we should assume it's being used in that way.

@hellofromtonya hellofromtonya force-pushed the try/webfonts-architecture-extend-wp_dependencies branch 2 times, most recently from c4fd65f to e55bab4 Compare August 11, 2022 21:41
@hellofromtonya hellofromtonya force-pushed the try/webfonts-architecture-extend-wp_dependencies branch from ba4847a to 1c4e7ea Compare August 18, 2022 11:53
Tonya Mork added 4 commits August 18, 2022 07:01
@hellofromtonya
Let PHPUnit Polyfills match PHPUnit version (#43334)
cb4b5ea 
Removes and gitignore composer.lock file
Removes PHPUnit dep from composer.json
@hellofromtonya
PHPUnit: turns on PHP notices and deprecations (#43102)
c181423
@hellofromtonya
Skips if individual style property not defined (#43122)
6d0ba62
@hellofromtonya
Exclude file and class name sniffs for PHPUnit tests (#43131)
e571a86 
PHPCS: Exclude PHPUnit tests from file and class name sniffs (for Core parity)
@hellofromtonya hellofromtonya force-pushed the try/webfonts-architecture-extend-wp_dependencies branch from defbd63 to e571a86 Compare August 18, 2022 12:30
@hellofromtonya
Copy link
Contributor Author

Okay, this PR is 99% ready. Needs more testing and a thorough review. But the commit history is a mess. And it needs to be rebased against trunk. That's my next task > clean up the commit history and transform into atomic commits.

@hellofromtonya
Copy link
Contributor Author

Closing in favor of PR #43492.

@hellofromtonya hellofromtonya mentioned this pull request Aug 22, 2022
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aristath aristath aristath left review comments

@desrosj desrosj desrosj left review comments

@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
None yet
Projects
None yet
Milestone
No milestone
3 participants
@hellofromtonya @desrosj @aristath