Revert "Font Library: Group fonts by source (#63211)" #65590
Conversation
|
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. |
vcanales
added
[Type] Regression
|
Size Change: +1.21 kB (+0.07%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
noisysocks
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
This PR appears to contain an unintended change.
The purpose is just to revert the font grouping in the global styles sidebar, so I don't think there's any need to change the installed-fonts.js file.
In fact, when comparing trunk with this PR, there are differences in the font library modal.
- The title has changed. Additionally, the order of the Theme section and the Custom (Installed Fonts) section has been swapped:
| trunk | This PR |
|---|---|
- When there are no fonts, the message "No fonts installed." appears in the Library tab, but this is not the case with this PR:
| trunk | This PR |
|---|---|
From what I understand, we should only need to make the following changes in the trunk:
Diff
diff --git a/packages/edit-site/src/components/global-styles/font-families.js b/packages/edit-site/src/components/global-styles/font-families.js
index 6a554b1363..b40ab0a543 100644
--- a/packages/edit-site/src/components/global-styles/font-families.js
+++ b/packages/edit-site/src/components/global-styles/font-families.js
@@ -61,12 +61,12 @@ function FontFamilies() {
) }
<VStack spacing={ 4 }>
- { themeFonts.length > 0 && (
- <VStack>
+ { [ ...themeFonts, ...customFonts ].length > 0 && (
+ <>
<Subtitle level={ 3 }>
{
/* translators: Heading for a list of fonts provided by the theme. */
- _x( 'Theme', 'font source' )
+ _x( 'Fonts', 'font source' )
}
</Subtitle>
<ItemGroup size="large" isBordered isSeparated>
@@ -76,18 +76,6 @@ function FontFamilies() {
font={ font }
/>
) ) }
- </ItemGroup>
- </VStack>
- ) }
- { customFonts.length > 0 && (
- <VStack>
- <Subtitle level={ 3 }>
- {
- /* translators: Heading for a list of fonts installed by the user. */
- _x( 'Custom', 'font source' )
- }
- </Subtitle>
- <ItemGroup size="large" isBordered isSeparated>
{ customFonts.map( ( font ) => (
<FontFamilyItem
key={ font.slug }
@@ -95,7 +83,7 @@ function FontFamilies() {
/>
) ) }
</ItemGroup>
- </VStack>
+ </>
) }
{ ! hasFonts && (
<VStack>
There is no agreement on the proposed revert. The discussion on #63505 is still ongoing and didn't make to a consensus. As such, I would kindly ask you to refrain from merging this PR until a shared solution has been identified and agreed upon, thanks. |
Thanks. The revert had conflicts, and this helps me narrow down what I missed when merging them. |
vcanales
force-pushed
the
revert-group-fonts-by-source
branch
from
f517c55
to
68bb1be
Compare
|
I understand that no consensus has been reached and the discussion needs to continue. I'm not also sure I fully agree with reverting font grouping. However, if we do not ship this PR, that is, if we maintain font grouping, users will experience UI changes in WP 6.7. If we find an ideal UI in the future and implement it, users will experience UI changes again, and they may be confused by the repeated changes. Considering that, it might make sense to revert the font grouping at the time of WP 6.7 and provide users with the same UI as before. |
| const hasInstalledFonts = | ||
| hasFonts || | ||
| baseFontFamilies?.theme?.length > 0 || | ||
| baseCustomFonts?.length > 0; |
There was a problem hiding this comment.
There may be cases where fonts are installed in a theme, but all fonts are disabled. In this case, the expected text is "No fonts activated" and the button text is "Manage fonts".
However, if we remove these codes, including hasInstalledFonts, the expected text does not appear:
See #63533 for the intention of these codes.
I don't think this PR should make any changes other than reverting the font grouping.
There was a problem hiding this comment.
Thanks for checking again; I think the revert is constrained to the grouping now.
These are my thoughts as well. We can keep the discussion going, but there's enough disagreement over the original change—implementing grouping—to warrant a reversal until we reach an agreement. |
I totally agree but this is true for any change in the UI. And there's a lot of them at every release that are then changed again and again in the following releases. Ideally it should not happen. Stability sholduohs be a priority for the UI but is this principle is true, then I would love to see it applied to all UI changes not just the ones some contributors don't like. |
|
For what is worth, reverting the UI to how it was before #63211 is a no-go for me. Of course, it's just my opinion as experienced contributor. |
vcanales
force-pushed
the
revert-group-fonts-by-source
branch
from
68bb1be
to
ef44d04
Compare
|
What do you all think about expanding the description for the Typography screen, adding context about the Fonts? Changing the current description to "Available fonts, typographic styles, and the application of those styles on site elements" would probably help clarify what is on this screen. 
We currently don't mention Fonts in this description, and adding it explains their presence on this screen without the need for additional labels or altering the Subtitle. |
This is a very good suggestion. Perhaps the description should have been updated when the Font panel was added. Once the description is updated, this seems to me to be the best approach for WP 6.7. |
|
Let's merge this PR. I fully understand that there are various opinions, we haven't reached a consensus yet, and we need to continue discussing it in the future. Considering the impact on users and future improvements, I think it makes sense to ship this PR at this time. @richtabor @afercia What do you think about the description update? |
We should not push something into core that introduces unfamiliar UI/UX patterns. We need to revert and revisit, rather than change something for millions of users that makes it more complicated, then end up changing it back in the next release. |
100%. Change must be worth the change. I'm in support of improving this area, but what is in trunk, proposed for 6.7, is not the ideal path forward. |
Yes, that's ok. I think we can drop the elements part: "Available fonts, typographic styles, and the application of those styles. |
@richtabor done |
|
Flaky tests detected in 1787780. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11071846322
|
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
* Revert "Font Library: Group fonts by source (#63211)" * update typography screen description * update screen description Co-authored-by: vcanales <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: richtabor <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 88d6723 |
@t-hamano would you like to create a new issue as a follow-up when you have a chance, please? Please mark it with the accessibility label. To me, this change doesn't solve the fundamental issue about clarity of the UI. As an user, I don't understand what the list of fonts is about. As I mentioned on this comment, this list needs to clearly communicate it's a list of active fonts and there may be more fonts available. In a way, the new description contributes to make the UI even more unclear because it explicitly says 'available fonts`, while the list of fonts may not show all the actually available fonts. In fact, when deactivating some fonts, the list shows only the active ones. Screenshot:
However, in my case I have 9 more fonts installed and available but the UI doesn't inform me about that at all. This UI is unclear as it doesn't provide useful information for users and needs to be iterated on. |
I think it would be a good idea to continue the discussion in #63505. It might be a good idea to update the issue title and description. There are already a lot of opinions written in #63505, so I would like to make use of them.
Is there a better explanation for how to solve this problem in WP 6.7? If so, we can backport it to WP 6.7. |
I will comment on the issue. |
…ress#65590) * Revert "Font Library: Group fonts by source (WordPress#63211)" * update typography screen description * update screen description Co-authored-by: vcanales <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: richtabor <[email protected]>
WordPress#65590)" This reverts commit 61c26c3.
This reverts commit e7bf3b6.
Todo
What?
Following up on this discussion on #63505, this PR reverts the changes introduced in #63211, which grouped fonts by their sources (theme and custom) in the Font Library.
Why?
Users only need to interact with active fonts within the Typography panel, regardless of whether they come from the theme or are custom additions.
How?
The implementation involves:
Testing Instructions
Global Styles Testing
Font Management
Screenshots
cc. @richtabor