Fluid typography: do not calculate fluid font size when min and max viewport widths are equal #57866
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
…e fluid typography config, do not calculate fluid typography values and return null. We could provide a fallback from the default values in gutenberg_get_typography_font_size_value() but I think it might be better to bypass returning clamp values altogether because fundamentally, the input is wrong.
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 ❔ phpunit/block-supports/typography-test.php |
|
Size Change: +5 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
ramonjd
commented
Jan 16, 2024
lib/block-supports/typography.php
Outdated
Comment on lines
413
to
415
| $linear_factor = ! empty( $linear_factor_denominator ) ? 100 * ( ( $maximum_font_size['value'] - $minimum_font_size['value'] ) / ( $linear_factor_denominator ) ) : 0; | ||
| $linear_factor_scaled = round( $linear_factor * $scale_factor, 3 ); | ||
| $linear_factor_scaled = empty( $linear_factor_scaled ) ? 1 : $linear_factor_scaled; |
There was a problem hiding this comment.
Would this be more readable?
if ( ! empty( $linear_factor_denominator ) ) {
$linear_factor = 100 * ( ( $maximum_font_size['value'] - $minimum_font_size['value'] ) / ( $linear_factor_denominator ) );
$linear_factor_scaled = round( $linear_factor * $scale_factor, 3 );
} else {
$linear_factor_scaled = 1;
}
$linear_factor_scaled = empty( $linear_factor_scaled ) ? 1 : $linear_factor_scaled;
There was a problem hiding this comment.
Slightly! That looks a bit more readable to me since it reduces a bit of the complexity in a single line 👍
I don't feel strongly about it either way, though!
There was a problem hiding this comment.
Thanks for getting a PR up so quickly!
While testing, I found that the fluid typography output felt a bit unexpected to me as the resulting font-size seemed to always be very far away from the one I entered into the block editor. For example, if I set a block's font-size to 48px, then with * 1, it never gets close to being 48px despite the viewport size. In practice, it winds up being 27.894px to 32.458px:
2024-01-16.16.12.46.mp4
Not calculate fluid typography values and return null.
Because of the above, I think I'd lean toward an early return actually, and skip fluid typography output when the min and max viewport are the same.
Separately to this PR, I noticed that it looks like Fluid typography rules currently aren't working for custom values within the block editor. From a bit of digging, it looks like maybe the fluid typography calls aren't happening within useBlockProps here 🤔 :
That last bug isn't a blocker for this PR, but just thought I'd mention it while I notice it.
I've run out of time to keep digging today, but happy to help out with this tomorrow!
Added a comment over on #56912 (review) where I think the issue was introduced. Not quite sure what the right fix is 🤔 |
I should have listened to my first commit 29cf213 I'll revert. Thanks for the quick test! |
ramonjd
force-pushed
the
fix/fluid-typography-same-viewport-values
branch
from
23baf65 to
29cf213
Compare
ramonjd
changed the title
andrewserong
approved these changes
Jan 16, 2024
|
Another one I owe you @andrewserong 🙏 |
This was referenced Jan 17, 2024
getdave
added
Backported to WP Core
|
✅ I updated this PR with the |
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?
When the same value is provided for min and max viewport widths in the fluid typography config, return early.
Kudos to @josephscott for catching this.
Core patch:
Why?
To avoid dividing by zero values. PHP will throw an error and, besides, the fluid typography clamp rule needs valid max and min viewport constraints.
Alternatives to this approach:
gutenberg_get_typography_font_size_value(), e.g.,1600 - 320.Testing Instructions
The error is triggered when the min and max viewport widths are equal.
Here is some test theme.json
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "typography": { "fluid": { "minFontSize": "16px", "maxViewportWidth": "1640px", "minViewportWidth": "1640px" }, "fontSizes": [ { "name": "heading", "slug": "heading", "size": "20px" } ] } }, }Check that font-sizes are not fluid.
Important
In the Editor things will be appear broken because, via
wp-admin/edit-form-blocks.phpCore is running theme.json through the Theme_Json class, which will call Corewp_typography functions.Here's the expected error:
PHP error in the editor
There's a Core patch you can run to get everything working.
E.g., if using
wp-envuse a.wp-env.override.jsonoverride file to point to the WordPress source{ "core": "../wordpress-develop/src", "plugins": [ "." ] }