List all the font families and font sizes from all the theme.json origins in the font picker #51488
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
|
Size Change: +100 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
youknowriad
approved these changes
Jun 14, 2023
|
Flaky tests detected in bac3d72. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5277901909
|
There was a problem hiding this comment.
This sounds good to me, too. I take it it's okay that a user wouldn't be able to remove a font family set by the theme?
A couple of other quick thoughts:
- In
useHasFontFamilyControlit currently checks if the first found font family by origin is empty. If thecustomarray is empty, and the theme array isn't, then with this PR, it sounds like it should returntrue. Would it be worth updating that function to use the same.concat()logic? - When concatenated, should the lists of fonts be sorted in any particular way? Currently it looks like they'll be sorted by origin, which might be unexpected depending on the circumstances.
Code-wise this approach otherwise looks good to me, but I didn't manage to get it working for me locally with a custom key added to the fontFamilies in the revisions entry in the database. It's highly likely that I made a mistake in my testing... do you have a set of steps to follow for testing that this is working as expected?
Since Riad has already approved, don't feel like you need to wait for me before merging, though! 🙂
Yes, the user will be able to remove a font family defined by the theme by removing it from the array. We don't have the UI for that yet but it will be available in the font library.
Good catch! I added the same logic to
Yes! Thanks for pointing this out. I added sorting base on the font family name if it's defined or the slug in case the name is missing.
You should just add some valid theme.json containing font families to the global styles post in the database. |
matiasbenedetto
changed the title
|
Ooops... auto-merge was activated, and when tests passed, it merged. Please report if you find something weird. |
No worries! Thanks for the extra info and updates, the code change looks good to me 🙂 |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
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?
List all the font families from all the theme.json origins in the font picker
Why?
Currently, the font family picker is listing only font families from custom theme.json origin (global styles) if it's defined.
How?
Concat all the font families available from all the origins.
Testing Instructions
settings.typography.fontFamiliesin the custom global styles. (The theme.json is stored in the database)Fixes: #51487