Font Library: fix font uninstallation #56762
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jffng
added
[Type] Bug
|
Size Change: +2 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is working as expected with the if ( 0 === response.errors.length ) check. I think it would be better to omit the other change because it could lead to confusion in cases where, for some reason, there are fonts in global styles that are not installed in the library.
Comment on lines
113
to
115
| .filter( ( f ) => | ||
| baseCustomFonts.some( ( bf ) => bf.slug === f.slug ) | ||
| ) // Ensures that the font actually exists in the library. |
There was a problem hiding this comment.
Do you think this is necessary? I think it could lead to some inconsistencies like not displaying a font that is still activated (in global styles) but not installed.
There was a problem hiding this comment.
I think it could lead to some inconsistencies like not displaying a font that is still activated (in global styles) but not installed.
This is what happens now / on trunk. Without this line — if the font uninstallation fails, it will still display the activated font in the typography sidebar.
There was a problem hiding this comment.
I see what you mean though, I'll remove it for now and find another way to address it if there are issues later.
There was a problem hiding this comment.
if the font uninstallation fails, it will still display the activated font in the typography sidebar.
I think that's correct, if the font was not uninstalled, it should still render because it will continue being active, right?
There was a problem hiding this comment.
Not quite — the way this is failing, is that the font gets uninstalled successfully on the server side, but not in global styles since that is handled client side and that's where the bug is. The result is the font family is uninstalled and deleted, but is still defined in global styles / shows up in the sidebar.
| @@ -229,7 +232,7 @@ function FontLibraryProvider( { children } ) { | |||
| // Uninstall the font (remove the font files from the server and the post from the database). | |||
| const response = await fetchUninstallFonts( [ font ] ); | |||
| // Deactivate the font family (remove the font family from the global styles). | |||
| if ( ! response.errors ) { | |||
| if ( 0 === response.errors.length ) { | |||
jffng
force-pushed
the
fix/font-uninstallation
branch
from
c449b0d
to
842f754
Compare
matiasbenedetto
approved these changes
Dec 5, 2023
|
Flaky tests detected in 842f754. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7103784108
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR fixes a bug in the
uninstallFontfunction of the FontLibraryProvider class.It also adds some logic to ensure that only custom fonts that are actually installed are ever rendered to the client.Why?
Fixes #56166
How?
In the uninstallFonts function, check the response errors array for length.
In the context, filter out fonts that are not installed.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast