Font Library: Improve 'No fonts installed' state when fonts are installed but not activated #63533
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
akasunil
added
[Type] Bug
|
Size Change: +27 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
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. |
t-hamano
reviewed
Jul 15, 2024
There was a problem hiding this comment.
Thanks for the PR!
To ideally resolve #63418, I think the following two things also need to be addressed:
The button that opens the modal dialog text is 'Add fonts'.
The button to open the modal dialog brings users to the 'Upload' tab. It would be best to bring users to the 'Library' tab.
In this PR, the text is correct when there are no fonts activated, but the button text is "Add fonts":
The button text should be "Manage fonts" and the tab that opens when you click the button should be "Installed fonts".
Additionally, the following scenarios need to be considered:
- No theme fonts
- Installed fonts are present
- All installed fonts are deactivated
In this case, the text should be "No fonts activated." and the button text should be "Manage fonts", but this is not the case in this PR.
| @@ -39,6 +44,7 @@ function FontFamilies() { | |||
| .sort( ( a, b ) => a.name.localeCompare( b.name ) ) | |||
| : []; | |||
| const hasFonts = 0 < customFonts.length || 0 < themeFonts.length; | |||
| const hasBaseFonts = baseFontFamilies?.theme?.length > 0; | |||
There was a problem hiding this comment.
This conditional is not enough to know if there are fonts installed but not activated. Currenty, this only checks if the theme provides fonts that are not activated but it doesn't check if there are custom fonts installed but they are not activated.
There was a problem hiding this comment.
You're right; it's likely possible that the only available fonts are custom ones that haven't been activated. I'll update the PR
t-hamano
approved these changes
Jul 18, 2024
|
Thank you @t-hamano
ill create separate issue for this. |
|
Also, the E2E test passes successfully in my local, here it fails. 
|
|
I don't know why the E2E tests succeed locally, I think it's because the button text should be "Manage Fonts" but it's "Add Fonts". The failing test is here, and below is a video of running the test manually. a02dd1590e7b24ea8f7597b36006b4f7.mp4To run the test manually, visit You will notice that the button text is unintentionally "Add Fonts". So maybe there's something missing in the current implementation. |
|
@t-hamano It works fine now Index-.-Template-.-gutenberg-.-Editor-.-WordPress.1.webm |
t-hamano
approved these changes
Jul 22, 2024
2 tasks
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?
Fixes #63418
Why?
Styles panel shows a 'No fonts installed.` text event when fonts are installed, but not activated.
How?
Added condition to display message conditionally. Now it will display 'No fonts activated.` when fonts are installed but not activated.
Testing Instructions