Font Library: Set filename using the font face data to the font assets #58474
Conversation
matiasbenedetto
added
[Type] Enhancement
matiasbenedetto
requested review from
pbking,
creativecoder and
mikachan
and removed request for
pbking
|
Size Change: +62 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0801519. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9746790720
|
There was a problem hiding this comment.
For Google Fonts, this is better than the strings of characters we have now in font file names.
However, this seems like an issue with Google fonts that should be solved (or worked around) with that collection, not something enforced for all collections.
For example, in the case of a font face with custom font properties, I might get a filename like this: noto-sans-mono-normal-400-swap-normal-0-0-normal-none-u-0000-00-ff.woff2 (and that's not even all the possible properties) -- that doesn't seem very good either.
I agree, I think this issue should be solved on the collection level. |
It's a valid point, but that would be an improbable case. Anyway, some of the values don't help identify what the file is about, so we can reduce the number of properties used to generate the filenames. Having 'fontFamily,' 'fontStyle,' 'fontWeight', and 'unicodeRange' seems enough to create meaningful filenames. We would get filenames like this after this commit 9737488
|
Having descriptive names on the files created seems valuable for the users, so I wonder why we should restrict the solution to only one font collection or let the collection decide that. I understand that all the font assets created, no matter what font collection they are coming from, should have informative filenames. It would like to have @colorful-tones input here as the creator of the issue #58411 |
| 'fontFamily', | ||
| 'fontStyle', | ||
| 'fontWeight', | ||
| 'unicodeRange', |
There was a problem hiding this comment.
Note that fontStretch is also needed here to be more in line with the css spec for font matching.
There was a problem hiding this comment.
Assuming these properties are concatenated to create the final Google Fonts file name only. Then I would suggest we leave out fontStretch. I don't think we need every property in the file name and it'd get looooonnnnnggg.
There was a problem hiding this comment.
We could slightly reduce the length of these filenames by using aliases for the font styles, something like:
- italic =
italorit - normal =
normornor - oblique =
obliorobl
It's only a tiny change but it may make a difference on particularly long file names.
We could also consider using pascal-case on the font family name (e.g. AROneSans rather than ar-one-sans, similar to how they're saved in the Google Fonts directory) to further reduce the filenames. The properties could still be separated by dashes, like AdLaMDisplay-normal-400.woff2 instead of ad-la-m-display-normal-400.woff2.
Edit: There's some prior art here if it's helpful, which uses [font-family]-[version]-[variant]-[font-weight][font-style], e.g. advent-pro-v28-latin-100italic.woff2. This is even longer, so maybe we don't need to worry about the filename length too much!
There was a problem hiding this comment.
Having 'fontFamily,' 'fontStyle,' 'fontWeight', and 'unicodeRange' seems enough
I agree. This adequately conveys the font properties in a file name while striking a balance and not adding every property.
Should both the manually uploaded font files and those added from a font collection, such as Google Fonts, be renamed?
No, we should not rename all font files. I would leave Uploaded (manually) fonts alone and only rename the Google Fonts collection.
I tested the following on this branch:
- Added several Google Fonts and variants (weights) and they all renamed nicely 👍
- Uploaded a font with the existing file name:
NEOPIXEL-Regular.otf, which resulted inwp-content/fonts/NEOPIXEL-Regular.otf - Next, I deleted the
wp-content/fonts/NEOPIXEL-Regular.otffrom the directory and in the Font Library. I then renamed theNEOPIXEL-Regular.otffile on my system to jibberish:asdfgasdfasdfasdf.otfand Uploaded it again and the file name persisted:wp-content/fonts/asdfgasdfasdfasdf.otf. I was not clear on whether the code introduced in this Pull Request would affect the Uploads and I wanted to report that it does not (in case it was expected). Do you mind clarifying this @matiasbenedetto please?
However, this seems like an issue with Google fonts that should be solved (or worked around) with that collection, not something enforced for all collections.
I would love to test with another collection to see it in action, but I need to figure out how? Without testing, it may be unique to the Google Fonts collection and would lean towards targeting that collection only.
I hate introducing further complexity or scope creep, but we might consider having named directories per collection.
wp-content/fonts/google-fonts/azeret-mono-italic-100.woff2wp-content/fonts/uploaded/NEOPIXEL-Regular.otfwp-content/fonts/another-collection/a-cool-font.woff2
I'm happy to open a follow-up issue for that, and I want to avoid causing noise by getting this changeset through. As is, this is an improvement, and I approve. Thanks @matiasbenedetto
|
@matiasbenedetto what is the latest status on this? Do we need more code reviewers? I did not mean to confuse anyone with my last feedback. Ultimately, I think this is a great addition (as is) and I think it is good to be merged. |
|
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. |
There was a problem hiding this comment.
This is working well in my testing, I can see the font filename is transformed into a readable name.
It would be good to add a couple of tests for the new makeFileNameFromFontFace function. I also left a comment about ways we could reduce the length of the filenames in the cases where they may be longer than normal.
| 'fontFamily', | ||
| 'fontStyle', | ||
| 'fontWeight', | ||
| 'unicodeRange', |
There was a problem hiding this comment.
We could slightly reduce the length of these filenames by using aliases for the font styles, something like:
- italic =
italorit - normal =
normornor - oblique =
obliorobl
It's only a tiny change but it may make a difference on particularly long file names.
We could also consider using pascal-case on the font family name (e.g. AROneSans rather than ar-one-sans, similar to how they're saved in the Google Fonts directory) to further reduce the filenames. The properties could still be separated by dashes, like AdLaMDisplay-normal-400.woff2 instead of ad-la-m-display-normal-400.woff2.
Edit: There's some prior art here if it's helpful, which uses [font-family]-[version]-[variant]-[font-weight][font-style], e.g. advent-pro-v28-latin-100italic.woff2. This is even longer, so maybe we don't need to worry about the filename length too much!
What?
Use the font face data to generate the font file asset name.
Why?
Fix: #58411
How?
Generating a filename based on the fontface definition.
Testing Instructions
Install a font face from a collection. Verify that the filename of the assets is related to the font face installed.