-
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
Refactored download/upload logic to support font faces with multiple src assets #58216
Conversation
Size Change: +2.75 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
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.
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. |
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Show resolved
Hide resolved
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.
This works when testing the happy path. However, I think there are some issues with the Promise error handling that need to be resolved.
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Show resolved
Hide resolved
For anyone else testing this, here is a test font collection with a multiple src font add_action( 'init', function() {
wp_register_font_collection(
array(
'slug' => 'multi-src-test',
'name' => 'Multi Source Test',
'categories' => array(
array(
'name' => 'Sans Serif',
'slug' => 'sans-serif'
),
),
'font_families' => array(
array(
'font_family_settings' => array(
'name' => 'Noto Sans Mono',
'fontFamily' => 'Noto Sans Mono, sans-serif',
'slug' => 'noto-sans-mono',
'fontFace' => array(
array(
'src' => array(
'https://fonts.gstatic.com/s/notosansmono/v30/BngrUXNETWXI6LwhGYvaxZikqZqK6fBq6kPvUce2oAZcdthSBUsYck4-_FNJ49_XVEQQL8Y.woff2',
'https://fonts.gstatic.com/s/notosansmono/v30/BngrUXNETWXI6LwhGYvaxZikqZqK6fBq6kPvUce2oAZcdthSBUsYck4-_GFJ49_XVEQQL8Y.woff2'
),
'fontWeight' => '400',
'fontStyle' => 'normal',
'fontFamily' => 'Noto Sans Mono',
'preview' => 'https://s.w.org/images/fonts/17.6/previews/noto-sans-mono/noto-sans-mono-400-normal.svg'
)
)
),
'categories' => array( 'sans-serif' )
)
)
)
);
} ); |
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 now tested error handling by both blocking network requests and using invalid font urls, and both are handled with an error message in the UI.
I left a couple of suggestions for simplifying comments and adding one about the lack of a catch method due to upstream error handling. Otherwise I think this is good to go.
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
Outdated
Show resolved
Hide resolved
…odal/utils/index.js Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>
…odal/utils/index.js Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>
…zy-hydration * origin/trunk: (47 commits) Interactivity API: Break up long hydration task in interactivity init (#58227) Add supports.interactivity to Query block (#58316) Font Library: Make notices more consistent (#58180) Fix Global styles text settings bleeding into placeholder component (#58303) Fix the position and size of the Options menu, (#57515) DataViews: Fix safari grid row height issue (#58302) Try a fix (#58282) Navigation Submenu Block: Make block name affect list view (#58296) Apply custom scroll style to fixed header block toolbar (#57444) Add support for transform and letter spacing controls in Global Styles > Typography > Elements (#58142) DataViews: Fix table view cell wrapper and BlockPreviews (#58062) Workflows: Add 'Technical Prototype' to the type-related labels list (#58163) Block Editor: Optimize the 'useBlockDisplayTitle' hook (#58250) Remove noahtallen from .wp-env codeowners (#58283) Global styles revisions: fix is-selected rules from affecting other areas of the editor (#58228) Try: Disable text selection for post content placeholder block. (#58169) Remove `template-only` mode from editor and edit-post packages (#57700) Refactored download/upload logic to support font faces with multiple src assets (#58216) Components: Expand theming support in COLORS (#58097) Implementing new UX for invoking rich text Link UI (#57986) ...
…src assets (#58216) Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>
I just cherry-picked this PR to the release/17.6 branch to get it included in the next release: d3f8d3a |
…src assets (#58216) Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>
What?
Refactored download/upload logic to support font faces with multiple src assets
Why?
Font Faces can provide multiple
src
values (for instance, providing both TTF and WOFF assets). These are provided as an array of strings instead of a string. Currently the user interface doesn't allow font faces with multiplesrc
assets to be installed. This change fixes that.Fixes #58173
How?
Checks the
src
attribute and makes it an array if it is a string, downloads all assets in the array of src values and uploads all of them in the call to create font faces.Testing Instructions
font-library.php
where the default font collection is added for testing)** Two font assets are downloaded
** Two font assets are uploaded in the create font face API call
** The create font face API call is successful