Fix list of base theme fonts when a theme variation is applied. #59959
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
… fonts in the font library modal. Avoid fonts with duplicated slugs.
matiasbenedetto
added
[Type] Bug
matiasbenedetto
requested review from
pbking,
scruffian,
mikachan and
creativecoder
|
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 was referenced Mar 18, 2024
|
Size Change: +300 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
getdave
reviewed
Mar 19, 2024
There was a problem hiding this comment.
I took this for a spin. What I saw was that despite the correct font name being shown when clicking on the that family in the sidar, the modal seemed to be showing a serif font in the preview when the font was supposed to be sans-serif.
It's unusual because the CSS font-family has Instrument Sans but the font doesn't seem to be applied.
Screen.Capture.on.2024-03-19.at.10-09-18.mp4
Ok thanks for clarifying. In which case the test case you provided works as described 👍 |
scruffian
reviewed
Mar 20, 2024
packages/edit-site/src/components/global-styles/font-library-modal/context.js
Show resolved
Hide resolved
scruffian
approved these changes
Mar 20, 2024
mikachan
approved these changes
Mar 21, 2024
packages/edit-site/src/components/global-styles/font-library-modal/context.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Following the steps on the video from #59818, I can see that this fixes the issue with clicking on a font in the sidebar list and then seeing different font in the modal (e.g. clicking on "Jost" and then seeing the settings for "Cardo").
Here's a before and after of the Font Library (trunk compared to this branch), after selecting the "Ember" theme variation for TT4
| Before | After |
|---|---|
 |
 |
One strange thing I'm seeing for the Ember variation is that System Serif is listed twice using this branch. Is that related, or a different issue?
Thanks for the detailed review. I've seen that. I found that one variation was using |
creativecoder
approved these changes
Mar 21, 2024
There was a problem hiding this comment.
This fixes the issue where the theme font list in the sidebar doesn't match the font list in the font library modal after selecting a theme variation or typography preset.
Tested with both WP trunk and WP 6.4 + Gutenberg plugin activated.
Thanks for fixing this! I'm not very familiar with this part of the code, but the comments certainly help explain it.
Note one small comment typo.
packages/edit-site/src/components/global-styles/font-library-modal/context.js
Outdated
Show resolved
Hide resolved
Can confirm it's the theme.json font slug typo that causes this, and it was fixed here: WordPress/wordpress-develop@b617b5c |
Co-authored-by: Grant Kinney <[email protected]>
|
@getdave @youknowriad This PR seems important to be included in the release candidate. Could we add it? |
getdave
added a commit
that referenced
this pull request
Mar 25, 2024
* use globalStyles + theme.json base files to render a list of avialble fonts in the font library modal. Avoid fonts with duplicated slugs. * add comments * improve comment * fix typo in comment Co-authored-by: Grant Kinney <[email protected]> --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]>
bph
added
the
Backport to Gutenberg RC
|
The fix works as expected for me, and I cannot foresee uncontrolled side-effects: it's contained to the font library modal, essentially. Besides the obvious, I've tested some edge cases, such as 1) declaring a font family in the theme.json (theme.json sans-serif) that wasn't defined in any variation 2) adding a font family in a variation (variation sans-serif) that wasn't defined anywhere else. This is how it works: Gravacao.do.ecra.2024-03-26.as.10.56.54.mov |
mcsf
reviewed
Mar 26, 2024
| * Avoids incosistencies with the fonts listed in the font library modal as base (unactivated). | ||
| * These inconsistencies can happen when the active theme fonts in global styles aren't defined in theme.json file as when a theme style variation is applied. | ||
| */ | ||
| const baseThemeFonts = baseFontFamilies?.theme |
There was a problem hiding this comment.
@matiasbenedetto: Since the value is now a union with themeFonts, are these still baseThemeFonts — or does the name need changing?
|
This PR was triaged by Editor release leads and contributors in WP Slack as part of the final WordPress 6.5 process. See https://wordpress.slack.com/archives/C02QB2JS7/p1711369960505499 (requires registration). The criteria used was:
Conclusion: we propose that this fix is included in WP 6.5. Reason: bug affects the (blessed) Font Library feature in a moderately-severe way and this PR resolves in a clear and traceable manner. We now await a final decision from the full release team. |
|
I just cherry-picked this PR to the release/18.0 branch to get it included in the next release: aab2adc |
bph
pushed a commit
that referenced
this pull request
Mar 26, 2024
* use globalStyles + theme.json base files to render a list of avialble fonts in the font library modal. Avoid fonts with duplicated slugs. * add comments * improve comment * fix typo in comment Co-authored-by: Grant Kinney <[email protected]> --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]>
bph
removed
the
Backport to Gutenberg RC
getdave
added a commit
that referenced
this pull request
Mar 27, 2024
* use globalStyles + theme.json base files to render a list of avialble fonts in the font library modal. Avoid fonts with duplicated slugs. * add comments * improve comment * fix typo in comment Co-authored-by: Grant Kinney <[email protected]> --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]>
This was referenced Mar 27, 2024
|
Looking at adding test coverage in #60250 |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…Press#59959) * use globalStyles + theme.json base files to render a list of avialble fonts in the font library modal. Avoid fonts with duplicated slugs. * add comments * improve comment * fix typo in comment Co-authored-by: Grant Kinney <[email protected]> --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]>
getdave
added
Backported to WP Core
|
I just cherry picked this into WP 6.5 RC 4 branch 975ee76 |
getdave
added a commit
that referenced
this pull request
Mar 27, 2024
* use globalStyles + theme.json base files to render a list of avialble fonts in the font library modal. Avoid fonts with duplicated slugs. * add comments * improve comment * fix typo in comment Co-authored-by: Grant Kinney <[email protected]> --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]>
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Mar 28, 2024
This merges several high priority bug fixes for the editor ahead of WordPress 6.5: - WordPress/gutenberg#60180 - WordPress/gutenberg#60093 - WordPress/gutenberg#60071 - WordPress/gutenberg#60130 - WordPress/gutenberg#59959 - WordPress/gutenberg#60167 Props youknowriad, annezazu, mcsf, jsnajdr, mmaattiiaass, get_dave, scruffian, mikachan, grantmkin, andraganescu, scruffian, antosguillamot, fabiankaegy, huzaifaalmesbah, krupajnanda, colorful-tones, liviopv, mamaduka, kim88, poena, peterwilsoncc, wildworks, swissspidy, desrosj, jorbin. Fixes #60315. git-svn-id: https://develop.svn.wordpress.org/trunk@57888 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Mar 28, 2024
This merges several high priority bug fixes for the editor ahead of WordPress 6.5: - WordPress/gutenberg#60180 - WordPress/gutenberg#60093 - WordPress/gutenberg#60071 - WordPress/gutenberg#60130 - WordPress/gutenberg#59959 - WordPress/gutenberg#60167 Props youknowriad, annezazu, mcsf, jsnajdr, mmaattiiaass, get_dave, scruffian, mikachan, grantmkin, andraganescu, scruffian, antosguillamot, fabiankaegy, huzaifaalmesbah, krupajnanda, colorful-tones, liviopv, mamaduka, kim88, poena, peterwilsoncc, wildworks, swissspidy, desrosj, jorbin. Fixes #60315. Built from https://develop.svn.wordpress.org/trunk@57888 git-svn-id: http://core.svn.wordpress.org/trunk@57389 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to gilzow/wordpress-performance
that referenced
this pull request
Mar 28, 2024
This merges several high priority bug fixes for the editor ahead of WordPress 6.5: - WordPress/gutenberg#60180 - WordPress/gutenberg#60093 - WordPress/gutenberg#60071 - WordPress/gutenberg#60130 - WordPress/gutenberg#59959 - WordPress/gutenberg#60167 Props youknowriad, annezazu, mcsf, jsnajdr, mmaattiiaass, get_dave, scruffian, mikachan, grantmkin, andraganescu, scruffian, antosguillamot, fabiankaegy, huzaifaalmesbah, krupajnanda, colorful-tones, liviopv, mamaduka, kim88, poena, peterwilsoncc, wildworks, swissspidy, desrosj, jorbin. Fixes #60315. Built from https://develop.svn.wordpress.org/trunk@57888 git-svn-id: https://core.svn.wordpress.org/trunk@57389 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Mar 28, 2024
This merges several high priority bug fixes for the editor ahead of WordPress 6.5: - WordPress/gutenberg#60180 - WordPress/gutenberg#60093 - WordPress/gutenberg#60071 - WordPress/gutenberg#60130 - WordPress/gutenberg#59959 - WordPress/gutenberg#60167 Reviewed by jorbin, swissspidy. Merges [57888] to the 6.5 branch. Props youknowriad, annezazu, mcsf, jsnajdr, mmaattiiaass, get_dave, scruffian, mikachan, grantmkin, andraganescu, scruffian, antosguillamot, fabiankaegy, huzaifaalmesbah, krupajnanda, colorful-tones, liviopv, mamaduka, kim88, poena, peterwilsoncc, wildworks, swissspidy, desrosj, jorbin. Fixes #60315. git-svn-id: https://develop.svn.wordpress.org/branches/6.5@57891 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Mar 28, 2024
This merges several high priority bug fixes for the editor ahead of WordPress 6.5: - WordPress/gutenberg#60180 - WordPress/gutenberg#60093 - WordPress/gutenberg#60071 - WordPress/gutenberg#60130 - WordPress/gutenberg#59959 - WordPress/gutenberg#60167 Reviewed by jorbin, swissspidy. Merges [57888] to the 6.5 branch. Props youknowriad, annezazu, mcsf, jsnajdr, mmaattiiaass, get_dave, scruffian, mikachan, grantmkin, andraganescu, scruffian, antosguillamot, fabiankaegy, huzaifaalmesbah, krupajnanda, colorful-tones, liviopv, mamaduka, kim88, poena, peterwilsoncc, wildworks, swissspidy, desrosj, jorbin. Fixes #60315. Built from https://develop.svn.wordpress.org/branches/6.5@57891 git-svn-id: http://core.svn.wordpress.org/branches/6.5@57392 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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?
Why?
Fixes part of: #59818
Fixes part of: #59574
How?
Using as a base theme fonts the list of fonts composed by the de-duplicated list of:
theme.jsonfile font familiesTesting Instructions
Screenshots or screencast
Before
Notice that the font selected in the sidebar is not the one displayed in the modal.
2024-03-18.13-19-04.mp4
After
2024-03-18.13-15-37.mp4