Font Library: adds the ability to use generic() in font family names. #59103
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
… CSS level 4 spec. Example: generic(kai)
|
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. |
|
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❔ lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php |
matiasbenedetto
added
[Type] Enhancement
|
Size Change: -1.35 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Looks like I the spaces should not be allowed inside generic() I'll remove that. |
Comment on lines
+37
to
39
| $regex = '/^(?!generic\([a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/'; | ||
| $item = trim( $item ); | ||
| if ( preg_match( $regex, $item ) ) { |
There was a problem hiding this comment.
Perhaps a simpler way to handle this part: use this regex that matches font family names that don't need quotes, and then add the quotes if there isn't a match: /^(generic\()?[A-z\-]+\)?$/
There was a problem hiding this comment.
there are some cases where this regex won't work as expected.
Example: generic(font will match it because in the regex the trailing (is optional.
There was a problem hiding this comment.
There's another error in the regex proposed that could produce unexpected results. A-z doesn't match the same characters as a-zA-Z but a wider and, in this case, unwanted set of characters.
Example: backets[]fontmatch that regex.
Reference: https://stackoverflow.com/questions/4923380/difference-between-regex-a-z-and-a-za-z
There was a problem hiding this comment.
Also, making the regex positive instead of negative makes it less specific and could match other unwanted strings, such as an empty string. For these reasons, I think we should keep the current regex.
There was a problem hiding this comment.
Good points, thanks for explaining. I see why the negative matching makes more sense in this case.
| // Matchs any non alphabetic characters (a-zA-Z), dashes - , or parenthesis () | ||
| const regex = /[^a-zA-Z\-()]+/; | ||
| // Matches strings that are not exclusively alphabetic characters or hyphens, and do not exactly follow the pattern generic(alphabetic characters or hyphens). | ||
| const regex = /^(?!generic\([ a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/; |
There was a problem hiding this comment.
Same as above, seems simpler to use /^(generic\()?[A-z\-]+\)?$/ and add quotes if a font family name doesn't match.
There was a problem hiding this comment.
See my answer above :)
|
Thanks for the review!
I think we should maintain the current, I explained the reasons here: #59103 (comment)
That's true 👍 |
|
Flaky tests detected in ea65057. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7931386181
|
creativecoder
approved these changes
Feb 16, 2024
Comment on lines
+37
to
39
| $regex = '/^(?!generic\([a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/'; | ||
| $item = trim( $item ); | ||
| if ( preg_match( $regex, $item ) ) { |
There was a problem hiding this comment.
Good points, thanks for explaining. I see why the negative matching makes more sense in this case.
youknowriad
added
[Type] Bug
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
…#59103) * adds the ability to use generic in font families as the examples from CSS level 4 spec. Example: generic(kai) * update comment
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 07eaea9 |
getdave
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
…#59103) * adds the ability to use generic in font families as the examples from CSS level 4 spec. Example: generic(kai) * update comment
creativecoder
added
[Type] Enhancement
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?
generic()in font family names following the examples from the CSS level 4 specification.Example:
Reference: https://www.w3.org/TR/css-fonts-4/#font-family-prop
Why?
To be compliant with the CSS spec.
How?
Not adding quotes around generic font families. Example
generic(kai)Testing instructions
Follow testing instructions from
generic()families.