Fluid typography: pass theme.json settings to override merged theme data #58362
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
ramonjd
added
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/typography.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ phpunit/block-supports/typography-test.php ❔ phpunit/class-wp-theme-json-test.php |
ramonjd
commented
Jan 29, 2024
ramonjd
commented
Jan 29, 2024
ramonjd
commented
Jan 29, 2024
ramonjd
commented
Jan 29, 2024
ramonjd
commented
Jan 29, 2024
|
Size Change: +1 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0417044. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7969791083
|
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
57167ab
to
713edfa
Compare
ramonjd
added
the
[Feature] Typography
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
3f6da4d
to
80b31ba
Compare
ramonjd
changed the title
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
80b31ba
to
35df3df
Compare
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @costasovo. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
35df3df
to
47cb65e
Compare
There was a problem hiding this comment.
This is testing great for me @ramonjd, and the code looks good!
I was mostly testing just before your last update, so I can give it a quick re-test with the presets included. But so far:
✅ Post editor output is as on trunk
✅ Site editor output is as on trunk
✅ Refactorings look good
✅ Output with provided PHP snippet is looking good, too
Nice work 👍
I'm grateful for the testing! Sorry you had to go through that before the presets update 🙏🏻 I think that change is okay - seems like a useful addition (and fixes the output too!) but I pinged for a second opinion. Keen to hear your thoughts as well. 🍺 |
andrewserong
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
Looking good to me, thanks for the updates!
Custom PHP presets handled with fluid:
Custom PHP presets handled with fluid set to false:
...but I pinged for a second opinion. Keen to hear your thoughts as well.
Overall I really like the idea here, it makes a lot of sense to me to allow passing in of custom settings, and the way you've handled it for backcompat sounds reasonable to me, too 👍
Good call on getting a second opinion, but overall this LGTM! ✨
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
a1bb1db
to
03a7b0d
Compare
ramonjd
commented
Feb 23, 2024
| @@ -1824,7 +1825,7 @@ protected static function get_settings_values_by_slug( $settings, $preset_metada | |||
| is_callable( $preset_metadata['value_func'] ) | |||
| ) { | |||
| $value_func = $preset_metadata['value_func']; | |||
| $value = call_user_func( $value_func, $preset ); | |||
| $value = call_user_func( $value_func, $preset, $settings ); | |||
There was a problem hiding this comment.
Not sure yet, but this might also provide a path towards CSS var resolution for layout values. So,
"settings": {
"appearanceTools": true,
"layout": {
"contentSize": "var(--wp--custom--layout--content-size)",
"wideSize": "1100px"
},
"typography": {
"fluid": {
"maxViewportWidth": {
"ref": "settings.layout.contentSize"
},
"minViewportWidth": "600px"
}
}
},See:
- Fluid typography: Numerical values shall support "ref" data type for better design synchronization e.g. typography.fluid.maxViewportWidth = "ref": "settings.layout.contentSize" #53525
- Typography: theme.json "layout" settings cannot be custom css variables or references, must be explicit values #55070
|
Tentative ping to cc @WordPress/gutenberg-core if folks have any opinions to the changes to class WP_Theme_JSON in this PR: TL;DR Passing theme.json See: |
…raphy_font_size_value function. This is so that any stylesheets generated by the Theme_JSON class can use that class's theme_json instance.
…size_value (PHP) and getTypographyFontSizeValue (JS) Updated doc comment
Light refactoring Added extra test to check for empty objects/arrays
Added test for the above
ramonjd
force-pushed
the
update/typography-supports-with-theme-json-settings
branch
from
0417044
to
73ae2ab
Compare
|
I'll go ahead and merge this one. If there's any contention about the approach then I'm happy revisit/revert. |
|
Sorry, I forgot to add co-contribs 😢 |
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Mar 13, 2024
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Mar 14, 2024
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 2, 2024
Fluid typography: pass theme.json settings to override merged theme data
Dev note for WordPress 6.6Some theme.json presets require custom logic to generate their values, for example, when converting font size preset values to The custom logic is handled by callback functions defined against the In WordPress 6.6, settings of the current In the case of font sizes presets, it fixes a bug whereby the callback function — |
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.
This is a proof of concept so far. Once an approach is settled, I'll:
getTypographyFontSizeValue)value_funcWhat?
Working on something that resolves #58135
Updates the second argument of
gutenberg_get_typography_font_size_value()to be a theme_json settings array.Add a backwards compatibility condition to handle booleans.
Why?
To get fluid typography settings,
gutenberg_get_typography_font_size_value()callsgutenberg_get_global_settings().gutenberg_get_global_settings()fetches merged theme.json data viaWP_Theme_JSON_Resolver_Gutenberg.However, to generate a stylesheet, theme.json settings are often passed directly to
WP_Theme_JSON_Gutenberg.Consider the following (modified) example provided by @costasovo in #58135
Because typography block supports looks at the global settings only, the font sizes and CSS vars will have a
clamp()value in the returned stylesheet if the current theme has fluid typography activated.This is despite the explicit "false" in
$themeJsonData.How?
Passing theme.json settings to
gutenberg_get_typography_font_size_value()as the second argument.These settings will override any global theme settings.
Currently the argument expects a boolean, so I've added a backwards compat check.
Testing Instructions
Using a theme that has activated fluid typography, add the above PHP snippet somewhere in the code (e.g., the theme's functions.php file) and log the output of
get_stylesheet().Check that you can override the global theme settings.
Because this PR touches all fluid typography functionality, we'll also need to test basic output:
For example, I've been comparing themes against trunk:
Also, run tests for glory!
JS
PHP
Testing Instructions for Keyboard
Screenshots or screencast