-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Try adding a ceiling to the calculated minimum font size #49247
Fluid Typography: Try adding a ceiling to the calculated minimum font size #49247
Conversation
Size Change: +1.84 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in 72a5feb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4486001305
|
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 addressing this! It's certainly an improvement over trunk on mobile screens, but maybe still needs a little adjustment for larger ones.
I tried the (admittedly a bit exaggerated) size of 20rem on a 1200px wide screen, and on trunk it's clamped to ~17.5rem whereas on this branch it's 12.25rem. On a 900px screen, trunk gives me 15.75 (which is starting to look grotesquely overlarge) but this branch goes all the way down to 6.5 which seems a little on the small side.
If we don't want a slightly higher ceiling, could the other numbers be changed in a way that preserves the proportions a bit more for wider screens?
Thanks for testing @tellthemachines, and good catch about the font-size at larger viewports!
I think the next step, then, might be to try tweaking either the middle part of the |
In 09ad429, I've had a play with reducing the default minimum viewport width, which seems to change the calculation for the mid screen sizes a bit, so that it's now a bit larger across that range of sizes. That seems to be a pretty good other variable to play with, so I'll continue tweaking next week. For the moment, I won't bother updating the tests while we're still playing around with values — I'll do a little more testing next week between this PR and trunk, too, to see if we can change how things are affected at the narrower viewports without otherwise changing things too much from trunk. |
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 starting to look at this @andrewserong
I think you're targeting the right place in the code — where the min value is calculated — since we're really only worried about custom font values.
Font values that have a defined min and max will just do what they do.
It's a tricky one to get right however 🤔
@@ -516,6 +516,14 @@ function gutenberg_get_typography_font_size_value( $preset, $should_use_fluid_ty | |||
) | |||
); | |||
|
|||
// Sets a ceiling for the minimum font size. | |||
$minimum_font_size_ceiling = gutenberg_get_typography_value_and_unit( | |||
'64px', |
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.
Do you think this could be added up near the default var declarations? E.g.,
$minimum_font_size_ceiling = '64px';
?
That way we can open it up for theme.json config later (if we decide that's a good thing to do of course!)
@@ -540,6 +548,12 @@ function gutenberg_get_typography_font_size_value( $preset, $should_use_fluid_ty | |||
if ( ! $minimum_font_size_raw ) { | |||
$calculated_minimum_font_size = round( $preferred_size['value'] * $default_minimum_font_size_factor, 3 ); |
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.
Yeah linear doesn't appear to cut it.
Maybe we need a plot a growth curve so that y
(the calculated min size) grows at different rates depending on the value of x
(the give custom size).
I'm no expert by the way, but I can try to play around with equations to see if we can get a good fit.
Simple exponential growth seems to work okay as well, e.g.,
$calculated_minimum_font_size = round( pow( $preferred_size['value'], $default_minimum_font_size_factor ), 3 );
It seems to scale better when the scale factor is bumped from 0.75
to 0.875
, which is the ratio of 14px/16px, that is, $default_minimum_font_size_limit / root size
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.
Simple exponential growth seems to work okay as well, e.g.,
Oh, very cool idea! Feel free to commit to this branch or fork if you think it'd help for us to have multiple approaches on the go. I reckon a non-linear curve is a good thing to try out 👍
I should have a bit of time before the end of the week to have a play with this if you don't beat me to it!
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! I'll have a play this week. 🍺
@beafialho @iamtakashi @henriqueiamarino not urgent, but if you have time I would love if you could test this PR and how it feels with regards to fluid typography "just working" out of the box. The easiest way to test might be to use http://gutenberg.run/49247 🙏 |
Besides testing a paragraph on a custom font size, I tried by setting a huge custom font size to the site title and navigation and it seems to work well! responsive.mp4Thank you for the heads up @jasmussen and to everyone else who worked on this. |
Hi folks! Using this PR as a base, I've thrown up an alternative PR #49707 that calculates minimum font sizes according to a log curve. Here's a test URL: http://gutenberg.run/49707 The curve ensures that the scale used to calculate minimum font size for custom font sizes will decrease as the custom font size grows larger. But because the scale isn't linear, the scale will start closer to 0.75 for smaller font sizes and taper out to 0.25 for larger font sizes. Example:
The major difference between this PR and mine is that I've removed the minimum font size ceiling. As font sizes get very very big, the scale will reduce down to 0.25. I considered it a good balance whereby incredibly huge font sizes, such as 250px++, won't be coerced to 64px, which may seem arbitrary to a designer who wishes to use such large sizes. However, there's still scope in my PR to include a ceiling for calculated min font sizes if folks think it's a good idea. |
Thanks for diving in @ramonjd! I think your idea of using a logarithmic / non-linear scale would be a better approach so that in narrower viewports the visual differentiation between larger font sizes is preserved. In this one, we just cap out at If I don't get to it today, I'll take it for a proper run through tomorrow! |
Thanks for working on this one, it's an important one! Really appreciate this landing soon. |
Thanks for the encouragement @jasmussen! From re-testing, I think that the work @ramonjd is doing looking into a logarithmic scale in #49707 will be more viable as it preserves the proportionality between different font-sizes. It's looking really good to me so far, so let's all give that one a test to see how it might work. For now (and for clarity so that folks can see which PR is in the lead), I'll close out this PR. We can always re-open if we decided to go with a ceiling for the minimum font size instead of a logarithmic scale. |
Yeah, it might be worth keeping this one in our back pockets if it turns out the font size scale is too big of a change. I can't see anything wrong with having a config entry that says: "When calculating min font sizes, make sure none are larger than 🍺 |
Sounds good. I'll be ready to press the "Reopen and comment" button if we decide go with this option in the end 😄 |
What?
A potential fix for #49162
In the fluid typography output, explore setting a ceiling for the calculated minimum font size. This is to ensure that on smaller viewports, the font-size never gets too big for the viewport.
The PR currently uses a hard-coded
64px
as the ceiling (corresponding to4rem
), which borrows from the minimum font size of thexx-large
font size preset from TwentyTwentyThree theme. My thinking here, is that if it's a good default for the TT3 theme to use, then perhaps it's a good default for the custom fluid typography rules, too.Note: It is totally fine if the approach in this PR isn't viable — the intention is to poke at the existing fluid typography formula and learn more about what we need it to do.
Why?
The current rules for custom font-sizes with
settings.typography.fluid
set totrue
intheme.json
is for the maximum font-size to be multiplied by0.75
to arrive at the minimum font-size. This works nicely for smaller headings, but the linear scale doesn't work well for really big font-sizes, as they'll still be too big in smaller viewports.The idea in this PR is to see whether adding a ceiling to the minimum font size could resolve the issue for the most-part in the shorter-term. From some quick testing, it seems to work fairly well, with
clamp()
magic taking care of adjusting the font-size in between the minimum and maximum values.Of course, this PR is also just a starting point for discussion — we can try out other ideas or go in a different direction with this.
How?
64px
/4rem
.Testing Instructions
settings.typography.fluid
intheme.json
(E.g. TwentyTwentyThree), add a bunch of paragraphs to a post or page using a variety of custom font sizes including really big ones like8rem
. For a long paragraph, try using the text from the linked issue:Loco brings you family recipes, fresh, simple, tasteful ingredients and a nice vibe.
4rem
work okay? If there are a multiple large font sizes that are different at wide viewports (e.g.8rem
next to6rem
), do these scale down pleasingly?For quicker testing, here is some test markup that includes a range of paragraphs:
Test markup of a variety of paragraph blocks at different custom font sizes
Testing Instructions for Keyboard
Screenshots or screencast
Using the test paragraph from the linked issue, the following shows the font-size at desktop screen size, and a 375px viewport:
Screengrab:
2023-03-22.14.24.24.mp4