-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changed installFont to installFonts so that multiple font families can be installed at once #59451
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…n be installed at once
aa6843b
to
3b30695
Compare
Size Change: +87 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
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.
@@ -214,7 +214,7 @@ function FontCollection( { slug } ) { | |||
} | |||
|
|||
try { | |||
await installFont( fontFamily ); | |||
await installFonts( [ fontFamily ] ); |
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.
await installFonts( [ fontFamily ] ); | |
await installFonts( fontFamily ); |
Only a small nitpick but I think I'd rather the installFonts
function handled converting arguments to an array (e.g. checking if fontFamiliesToInstall
is an array, if not then converting it), as I think it would slightly improve the readability here.
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'm generally opposed to passing different types of arguments into a function. If we need to change how that value is passed to offer clarity I would rather do that (i.e. fontFamiliesToInstall = []; fontFamiliesToInstall.push( fontFamily ); installFonts( fontFamiliesToInstall );
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.
Yeah I think that's a good alternative too 👍
This was what I spent 90% of the time working on for this feature. :/ I thought I had it lined up but I'll take another look. |
Made a silly async error (and an even sillier length check). The result was that it worked fine when I had breakpoints. :P Less so otherwise. It is fixed now. Another look would be appreciate. :) |
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.
…n be installed at once (#59451) * Changed installFont to installFonts so that multiple font families can be installed at once --------- Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: provenself <provenself@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 122d14c |
…n be installed at once (#59451) * Changed installFont to installFonts so that multiple font families can be installed at once --------- Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: provenself <provenself@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
What?
Refactor installation logic to not just install multiple font faces at once, but multiple font families.
This is done so that any combination of font faces can be dropped onto the upload panel to install then all-at-once.
Why?
Fixes: #58689
Fixes: #59138
How?
Refactor installFont to support a collection of fontFamiles, all of which will be installed in a single user action
Testing Instructions
Use the upload panel to upload font faces from different font families in a single action.
All of those font faces should be installed.
Testing Instructions for Keyboard
Screenshots or screencast