-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fluid typography: allow individual preset overrides #7247
Fluid typography: allow individual preset overrides #7247
Conversation
…graphy is disabled or not active but an individual font preset does enable it, calculate the clamp value for that individual preset. Individual font sizes may opt-out of fluid typography if it is turned on globally. This commit does the opposite: individual font size presets can opt-in to fluid typography if it is not turned on globally.
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -359,7 +360,11 @@ public function data_generate_font_size_preset_fixtures() { | |||
'font_size_preset' => array( | |||
'size' => null, | |||
), | |||
'settings' => null, | |||
'settings' => array( | |||
'typography' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing an omission - here the test is checking whether a null
value is returned even when fluid typography is enabled.
@@ -429,8 +434,7 @@ public function data_generate_font_size_preset_fixtures() { | |||
), | |||
'returns already clamped value' => array( | |||
'font_size_preset' => array( | |||
'size' => 'clamp(21px, 1.313rem + ((1vw - 7.68px) * 2.524), 42px)', | |||
'fluid' => false, | |||
'size' => 'clamp(21px, 1.313rem + ((1vw - 7.68px) * 2.524), 42px)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing an omission - here the test is checking whether the clamped value is returned when fluid typography is enabled.
@@ -442,8 +446,7 @@ public function data_generate_font_size_preset_fixtures() { | |||
|
|||
'returns value with unsupported unit' => array( | |||
'font_size_preset' => array( | |||
'size' => '1000%', | |||
'fluid' => false, | |||
'size' => '1000%', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, addressing an omission - here the test is checking whether the unsupported unit is returned when fluid typography is enabled.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR and the extra inline explanations @ramonjd 👍
✅ Code changes align with corresponding Gutenberg PR
✅ Unit test updates look good and pass
✅ Tests as expected
The presets that have fluid settings should have fluid font sizes in the editor and the frontend.
This test step seems to conflict with the note below it that the changes only affect the frontend. I'm assuming it's just a copy pasta from the Gutenberg PR.
On the frontend only, I see the fluid font sizes:
![Screenshot 2024-08-28 at 12 16 32 PM](https://private-user-images.githubusercontent.com/60436221/362143648-129b447e-e65d-4b34-b85c-d8cd7a39e7a2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0MTI3ODEsIm5iZiI6MTczOTQxMjQ4MSwicGF0aCI6Ii82MDQzNjIyMS8zNjIxNDM2NDgtMTI5YjQ0N2UtZTY1ZC00YjM0LWI4NWMtZDhjZDdhMzllN2EyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEzVDAyMDgwMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTUzZTE4ZTIzYzljN2EyZDc5MmIzNTllNmE2MTlkMzQzYTYxMTRjMzEzOGM4MzkwMzQ0MGM1ZTZkM2E4YjgxYTUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.6w3MG_HAlLbiiLJJFadtY9XngIDcxVM7ejiBF6wZlLU)
I left a couple of minor nits/questions but all in all this is looking pretty good to me.
@@ -518,6 +518,7 @@ function wp_get_computed_fluid_typography_value( $args = array() ) { | |||
* @since 6.3.0 Using layout.wideSize as max viewport width, and logarithmic scale factor to calculate minimum font scale. | |||
* @since 6.4.0 Added configurable min and max viewport width values to the typography.fluid theme.json schema. | |||
* @since 6.6.0 Deprecated bool argument $should_use_fluid_typography. | |||
* @since 6.7.0 Enabled individual block fluid typography settings overrides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's just my lack of context here but this comment isn't very clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I'll update.
// $typography_settings['fluid'] can be a bool or an array. Normalize to array. | ||
$fluid_settings = is_array( $typography_settings['fluid'] ) ? $typography_settings['fluid'] : array(); | ||
$fluid_settings = isset( $typography_settings['fluid'] ) ? $typography_settings['fluid'] : array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here doesn't seem to match the code. Should it still be normalizing to an array or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ach, the comment needs to go. Good catch.
The change to isset
here prevents warnings if the fluid
property doesn't exist.
No "normalizing" isn't necessary as there are isset
checks on all potential properties of $fluid_settings
anyway.
Thanks for testing! I'll update those small things.
You assume correctly 😄 I'll update. |
This PR syncs the following Gutenberg PR:
cc @matiasbenedetto
What?
This PR allows individual font preset overrides.
So, if fluid typography is disabled or not active but an individual font preset does enable it, calculate the clamp value for that individual preset.
Why?
Individual font sizes may opt out of fluid typography if it is turned on globally.
This PR does the opposite: individual font size presets can opt in to fluid typography if it is not turned on globally.
How?
Check if fluid settings on individual font size presets are present.
Testing Instructions
"fluid": true
to the preset object. See the below example theme.json.Note
This only affects the frontend site, not the editor.
Here is some test theme.json
Trac ticket: https://core.trac.wordpress.org/ticket/61932
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.