Try/font family optionally upload assets #57989

pbking · 2024-01-19T00:51:13Z

What?

Update the Font Collection object to contain a require_download property
Add an editor setting of fontLibraryAssetInstall
Install (download, then upload) fonts when appropriate
Use hosted font face assets when appropriate
Hide Tabs (upload or specific font collections) when appropriate

The fontLibraryAssetInstall can be set to the values:

require : Require all Font Faces to be installed. This is the default. All Font Faces will always be installed.
allow : Allow Font Faces to be installed. Font Collections that can be hosted (require_download === FALSE) will be hosted, Font Collections that must be installed (require_download === TRUE) will be installed.
denied: Font Faces are not allowed to be installed into the system. The UPLOAD tab will be hidden. Any Font Collections that require installation (require_download === TRUE) will not be shown.

The value of fontLibraryAssetInstall can be set like this in PHP:

function set_font_library_asset_install_value( $editor_settings ) { 
	$editor_settings['fontLibraryAssetInstall'] = 'denied';
    return $editor_settings; 
}

add_filter( "block_editor_settings_all", "set_font_library_asset_install_value" );

Each Font Collection require_download property is set when the collection is created.

$default_font_collection = array(
	'slug'             => 'default-font-collection',
	'require_download' => false,
	'name'             => 'Google Fonts',
	'description'      => __( 'Add from Google Fonts. Fonts are copied to and served from your site.', 'gutenberg' ),
	'src'              => 'https://s.w.org/images/fonts/16.7/collections/google-fonts-with-preview.json',
);

Testing Instructions

When fontLibraryAssetInstall is denied
the UPLOAD tab is hidden
any collection with require_download set to TRUE is hidden

When fontLibraryAssetInstall is required:
the UPLOAD tab is available
All font assets are installed from any font collection

When fontLibraryAssetInstall is allowed:
the UPLOAD tab is available
any font faces installed from a collection with require_download set to TRUE is installed
any font faces installed from a collection with require_download set to FALSE is NOT installed

NOTE: While working on this I noticed that the calls to load individual Font Collections was unnecessary (that data is loaded when all of the font collections are loaded) and has been removed.

…API controller (#57656)

…d errors (#57843)

)

…arent (#57867) Co-authored-by: Sarah Norris <[email protected]>

…ised API (#57844) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <[email protected]> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) --------- Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Jason Crist <[email protected]>

* Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <[email protected]> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) * moved response processing into the resolver for fetchGetFontFamilyBySlug * Moved response processing for font family installation to the resolver * Refactored font face installation process to handle errors more cleanly * Cleanup error handling for font library view * Add i18n function to error messages * Add TODO comment for uninstall notice --------- Co-authored-by: Jeff Ong <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Sarah Norris <[email protected]>

…ngs (#57903)

* Fix delete endpoint * Update fetchUninstallFontFamily to match new format * Update uninstallFont * Add uninstall notice back in * Tidy up comments * Re-word uninstall notices * Add spacing to error message * Refactored how font family values were processed so they would retain their id, which is now used to delete a font family without fetching data via slug * Rename uninstallFont to uninstallFontFamily * Throw uninstall errors rather than returning them --------- Co-authored-by: Jason Crist <[email protected]>

…odal/local-fonts.js Co-authored-by: Jonny Harris <[email protected]>

…fontLibraryAssetInstall and a font collection's require_download value

github-actions · 2024-01-19T00:51:25Z

Warning: Type of PR label mismatch

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] 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] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
Labels found: [Feature] Typography.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

jffng

This tests as expected according to the instructions. I have a few comments:

I'm pretty confused at the naming. (It's a tricky one!) In particular — fontFamilyAssetInstall = allow and require_download = false means that the font face's src will be copied from the collection, as is. That feels pretty opaque.
If we do go with one editor setting with three options, could the names be present tense and consistent? I.e. allow, deny, require.
I think this will conflict with Font Library: font collection refactor to use the new schema #57884, particularly the removal of the getFontCollection function, so perhaps that one should come in first.

jffng · 2024-01-19T08:49:33Z

packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js

-		const fontFamily = fontsToInstall[ 0 ];
+		let fontFamily = fontsToInstall[ 0 ];
+
+		//NOTE: This is only necessary while the collection potentially includes the depreciated 'downloadFromUrl' property.


Suggested change

//NOTE: This is only necessary while the collection potentially includes the depreciated 'downloadFromUrl' property.

// TODO: Remove mapping once 'downloadFromUrl' property is deprecated.

jffng · 2024-01-19T09:01:10Z

packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js

+				return fontFace;
+			} );
+		}
+		////////////


Is this to indicate what chunk of code can be removed once downloadFromURL is deprecated? I think it's clear enough without it.

jffng · 2024-01-19T09:09:18Z

packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js

@@ -153,34 +140,67 @@ function FontCollection( { id } ) {
 		setFontsToInstall( [] );
 	};

+	const handleFontDownload = async ( fontFamily ) => {
+		return Promise.all(


Based on the last discussion we had about async, does this need to await?

Suggested change

return Promise.all(

return await Promise.all(

matiasbenedetto · 2024-01-19T14:23:02Z

packages/edit-site/src/components/global-styles/font-library-modal/resolvers.js

-export async function fetchFontCollection( id ) {
-	const config = {
-		path: `/wp/v2/font-collections/${ id }`,
-		method: 'GET',
-	};
-	return apiFetch( config );
-}


Why is this function being removed?

Ohh I found why in this comment:

NOTE: While working on this, I noticed that the calls to load individual Font Collections were unnecessary (that data is loaded when all of the font collections are loaded) and have been removed.

I think that should not be like that because the initial /font-collections/ request would be unnecessarily heavy.

creativecoder and others added 15 commits January 12, 2024 13:37

Font Library: add wp_font_face post type and scaffold font face REST …

c8072d4

…API controller (#57656)

Font Library: create font faces through the REST API (#57702)

61da35c

Refactor Font Family Controller (#57785)

42565a3

Font Family and Font Face REST API endpoints: better data handling an…

afacdf2

…d errors (#57843)

Font Families REST API endpoint: ensure unique font family slugs (#57861

7c82cb9

)

Font Library: delete child font faces and font assets when deleting p…

f84cc6d

…arent (#57867) Co-authored-by: Sarah Norris <[email protected]>

Font Faces endpoint: prevent creating font faces with duplicate setti…

8a5efd4

…ngs (#57903)

Update packages/edit-site/src/components/global-styles/font-library-m…

c6e0fbb

…odal/local-fonts.js Co-authored-by: Jonny Harris <[email protected]>

Merge branch 'trunk' into try/font-library-refactor

74be330

Removed unnecessary get font collection calls

d901381

add require_download property to font collections

1128b03

Add fontLibraryAssetInstall editor setting. Change the view based on …

a8779d8

…fontLibraryAssetInstall and a font collection's require_download value

pbking requested a review from spacedmonkey as a code owner January 19, 2024 00:51

jffng reviewed Jan 19, 2024

View reviewed changes

matiasbenedetto reviewed Jan 19, 2024

View reviewed changes

matiasbenedetto mentioned this pull request Jan 19, 2024

Font Library: add a setting to customize the conditional download of font assets from a font collection #58030

Open

creativecoder force-pushed the try/font-library-refactor branch from 5d160f2 to 1320d20 Compare January 22, 2024 15:44

Base automatically changed from try/font-library-refactor to trunk January 23, 2024 21:44

creativecoder mentioned this pull request Jan 25, 2024

Font Library: settings for using local vs. remotely hosted fonts #58229

Open

skorasaurus added the [Feature] Typography Font and typography-related issues and PRs label Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try/font family optionally upload assets #57989

Try/font family optionally upload assets #57989

pbking commented Jan 19, 2024

github-actions bot commented Jan 19, 2024 •

edited

Loading

jffng left a comment

jffng Jan 19, 2024

jffng Jan 19, 2024

jffng Jan 19, 2024

matiasbenedetto Jan 19, 2024

matiasbenedetto Jan 19, 2024 •

edited

Loading

	//NOTE: This is only necessary while the collection potentially includes the depreciated 'downloadFromUrl' property.
	// TODO: Remove mapping once 'downloadFromUrl' property is deprecated.

Try/font family optionally upload assets #57989

Are you sure you want to change the base?

Try/font family optionally upload assets #57989

Conversation

pbking commented Jan 19, 2024

What?

Testing Instructions

github-actions bot commented Jan 19, 2024 • edited Loading

jffng left a comment

Choose a reason for hiding this comment

jffng Jan 19, 2024

Choose a reason for hiding this comment

jffng Jan 19, 2024

Choose a reason for hiding this comment

jffng Jan 19, 2024

Choose a reason for hiding this comment

matiasbenedetto Jan 19, 2024

Choose a reason for hiding this comment

matiasbenedetto Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2024 •

edited

Loading

matiasbenedetto Jan 19, 2024 •

edited

Loading