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.

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

Font Library: Font Collection backend #53816

Closed
wants to merge 52 commits into from
Closed

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Aug 18, 2023

What?

Add extensibility capabilities to the Font Library.
Provides a way to provide collections of typographic fonts by code.

Why?

  • Standardize the way to add font collections to the font library.
  • Allows extenders code (plugins, for example) to register lists of fonts to let the user install the ones they want. Font foundries, for example, will be able to create a plugin to provide their fonts with just a few lines of code.

Example:

This is all the code required to create a plugin providing a font collection.

<?php
/*
Plugin Name: My Font Collection
Plugin URI: https://your-website.com/
Description: Add a font collection to your WordPress Font Library.
Version: 1.0
Author: Your Name
Author URI: https://your-website.com/
License: GPLv2 or later
Text Domain: my-font-collection
*/

$my_config = array (
    'id'             => 'my-font-collection',
    'name'           => 'My Font Collection',
    'description'    => 'Demo about how to a font collection to your WordPress Font Library.',
    'data_json_file' => path_join( __DIR__, 'my-font-collection-data.json' ),
);

if ( function_exists( 'wp_register_font_collection' ) ) {
    wp_register_font_collection ( $my_config );
}

?>

Download the plugin as zip ready to test:
my-font-collection.zip

How?

  • Implementation of a WP_Font_Collection class.
  • Implementation of the wp_register_font_collection filter.
  • Implementation of the /fonts/collections and fonts/collections/<id> endpoints.

To do

Tracking issue:

Stage 1: #52698
Stage 2: #53307

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

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.

  • Type-related labels to choose from: [Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Feature] Extensibility, [Feature] Typography.

Read more about Type labels in Gutenberg.

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/fonts/font-library/class-wp-font-collection.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/getData.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollections.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/base.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/getFontCollections.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/registerRoutes.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/uninstallFonts.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ lib/experimental/fonts/font-library/class-wp-rest-font-library-controller.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ lib/load.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/base.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/installFonts.php

@matiasbenedetto matiasbenedetto added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Typography Font and typography-related issues and PRs labels Aug 18, 2023
@matiasbenedetto matiasbenedetto removed the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Aug 18, 2023
@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Flaky tests detected in 60997e9.
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/5979891576
📝 Reported issues:

@matiasbenedetto matiasbenedetto changed the title Font Library: Adding font collection backend Font Library: Font Collection backend Aug 18, 2023
@spacedmonkey
Copy link
Member

If we must use static, then follow the pattern in wp_metadata_lazyloader that creates a instance to a static variable.

@matiasbenedetto
Copy link
Contributor Author

If we must use static, then follow the pattern in wp_metadata_lazyloader that creates a instance to a static variable.

@spacedmonkey Thanks for the review!
Could you please add more context about why is that pattern preferred over static methods and properties?

matiasbenedetto and others added 2 commits August 24, 2023 15:56
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@hellofromtonya
Copy link
Contributor

The source code LTGM and is ready 👍

Next: the tests need updating for the changes and then a code review. And then this PR is ready for merge.

Almost over the finish line with this PR.

@matiasbenedetto
Copy link
Contributor Author

I updated the plugin demo code in the PR description and the plugin's zip file based on the latest PR changes.

@matiasbenedetto
Copy link
Contributor Author

I created a follow-up of this PR to load data from URLs. It depends on this one: #53992

* Renamed files to remove `-test` suffix (no longer needed).
* Removed extra empty line before closing the class.
* Used Core's `assertWPError()`.
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

The source code LGTM 👍

The tests are good enough. I'll follow-up with a test improvement PR.

This PR is ready (assuming the CI jobs are happy).

@hellofromtonya hellofromtonya enabled auto-merge (squash) August 31, 2023 18:31
@hellofromtonya hellofromtonya removed the request for review from spacedmonkey August 31, 2023 19:12
@matiasbenedetto
Copy link
Contributor Author

Thanks, @hellofromtonya, for your work here!

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented Aug 31, 2023

I'm closing this PR because it's blocked by the changes requested by @spacedmonkey around the static class properties. I asked for more details about why we would need to avoid that pattern in this PR, and @hellofromtonya added some input about why the usage of the static property are intentional. Two weeks later, we didn't receive an answer, so I assume that the reasons presented are valid. This is blocking us from continuing the work on Font Library intended to be included in WordPress 6.4 release.

Because of that, I'm closing this PR and opening a new one. Of course, we are free to further discuss this topic on new issues, PRs, the make blog, or slack.

auto-merge was automatically disabled August 31, 2023 20:26

Pull request was closed

@hellofromtonya hellofromtonya deleted the try/font-collections branch August 31, 2023 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Typography Font and typography-related issues and PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants