Font Library: fix fonts not displaying correctly #55393
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: +670 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
jffng
force-pushed
the
fix/font-family-string-output
branch
from
8c7ed25
to
ec565b8
Compare
|
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❔ phpunit/tests/fonts/font-library/wpFontFamilyUtils/formatFontFamily.php ❔ lib/experimental/fonts/font-library/class-wp-font-family-utils.php ❔ lib/experimental/fonts/font-library/class-wp-font-family.php |
|
Another approach could be to ensure that all font family slugs get |
I'm still learning exactly how font collections and related plumbing work, but addressing this "closer to the source" with the goal of there needing to be less places in code that are concerned manipulating the font family slug and/or name seems better to me. |
mikachan
reviewed
Nov 15, 2023
There was a problem hiding this comment.
I've tested this with some of the font families mentioned in #54706 - Gothic A1, Libre Barcode 128 Text, Press Start 2P. They all worked well and I can see both the preset and the font family names are as you describe they should be with this PR.
However, I also agree with your follow-up comment and @creativecoder's, in that it may be best to solve this closer to the source and hopefully have fewer places where the font names and slugs are manipulated.
But I did want to confirm that this fix works 👏
jffng
force-pushed
the
fix/font-family-string-output
branch
from
d15ab3c
to
c102315
Compare
|
@mikachan @creativecoder I altered the approach, would you be able to give this another review? |
mikachan
reviewed
Nov 17, 2023
There was a problem hiding this comment.
Tested on a fresh install with:
- Gothic A1:
--wp--preset--font-family--gothic-a-1 - Libre Barcode 128 Text:
--wp--preset--font-family--libre-barcode-128-text - Press Start 2P:
--wp--preset--font-family--press-start-2-p - Rubik 80s Fade:
--wp--preset--font-family--rubik-80-s-fade
However, I'm seeing an intermittent issue where I need to refresh the editor in order for the newly installed fonts to apply correctly. I can see the CSS variables are saved and applied correctly, and the font file is loaded in the network tab, but the font isn't applied. If I save my changes and reload, the font is applied. It seems to happen with any font, not just fonts like the above. Do you know if the reload issue is related to this PR?
Edit: I've just managed to reproduce the reload issue on trunk, and found this issue is already reported 🙈 #55018. In that case, I think this PR is working well!
mikachan
approved these changes
Nov 17, 2023
| $this->data = $sanitized_font; | ||
|
|
||
| // Ensure slugs are kebab-cased and font families are wrapped in quotes. | ||
| if ( isset( $sanitized_font['slug'] ) ) { |
There was a problem hiding this comment.
Could this be a method?
There was a problem hiding this comment.
@matiasbenedetto I moved this formatting functionality to utils, what do you think? efa7602
There was a problem hiding this comment.
Tha's better. I reduced the function's scope to only one thing instead of 2 and renamed it accordingly: a414e3f
There was a problem hiding this comment.
We would need to add PHP unit tests to the function.
|
Flaky tests detected in 84ba302. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7007757782
|
derekblank
pushed a commit
that referenced
this pull request
Dec 7, 2023
* Add kebab casing and font family wrapping in sanitization function. Remove commented code. * Simplify how kebab casing and quote wrapping is done. * Ensure incoming font is returned. * Add kebab casing to slug in form data. * Add tests for wpKebabCase * remove redundant test * test 2 different inputs on first wpKebabCase case * Move formatting functions to utils. * reduce the scope of the function * fix the live preview (without reloading the page) of the just installed fonts in the editor * format php * Add test for format_font_family. * Format php. * Add more test cases and format. * Remove accidental badKey. * Format. --------- Co-authored-by: Vicente Canales <[email protected]> Co-authored-by: Sarah Norris <[email protected]> Co-authored-by: Sarah Norris <[email protected]> Co-authored-by: Matias Benedetto <[email protected]>
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.
This PR fixes #54706.
Background
There are two issues occurring, specific to font families with numbers and spaces inside their names / slugs:
Solution
Alter the
sanitizemethod of the Font Family Class to:_wp_to_kebab_casePart of the solution includes a small bug fix in the
getIntersectingFontFaces, as well as adding a test case.How to test
See #54706 — test all reported fonts, in the site editor, post editor, and front end. Verify they appear as expected.