-
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: use logarithmic scale factor to calculate a min font size #49707
Fluid typography: use logarithmic scale factor to calculate a min font size #49707
Conversation
349bc5a
to
430b09f
Compare
Size Change: +56 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Flaky tests detected in f5630e1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4728168606
|
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 trying this out! It's working well in testing with a range of different font sizes; the sizes scale very nicely while keeping proportionality between them.
The approach seems sound, my only question below is about changing the function parameters.
@@ -51,7 +49,6 @@ export function getComputedFluidTypographyValue( { | |||
minimumViewPortWidth = DEFAULT_MINIMUM_VIEWPORT_WIDTH, | |||
maximumViewPortWidth = DEFAULT_MAXIMUM_VIEWPORT_WIDTH, | |||
scaleFactor = DEFAULT_SCALE_FACTOR, | |||
minimumFontSizeFactor = DEFAULT_MINIMUM_FONT_SIZE_FACTOR, |
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.
Will removing this parameter cause any back compat issues, given that getComputedFluidTypographyValue
is part of the public block editor API?
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 raising that. Yeah, it's possible.
The minimumFontSizeFactor
was a value only configurable in the JS function getComputedFluidTypographyValue()
In the PHP, the size factor was hardcoded, and was never an option that could be passed: https://github.com/WordPress/gutenberg/pull/49707/files#diff-2c54d3c8305590cf826f83511a9fd85d5b405470addd62edc3183ff9118177fbL486
In that way, this PR harmonizes the front and backend code 😅 I guess the question is whether there are JS usages out there where one would expect it work as before (in the editor at least).
But my guess is that it'd be relatively safe given that the backend code never allowed customization of this value.
Furthermore, I've removed minimumFontSizeFactor
from the JS completely in this PR so passing it won't cause an error.
I can't find any explicit usages on https://wpdirectory.net/, but happy to follow folks' advice if there are backwards compat alarm bells.
Besides this, in general, this PR does represent a change in the way the min values are calculated and therefore has the potential to change the way a site's typography behaves. Also it changes the default min viewport, which will do the same.
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.
Great work @ramonjd, as @tellthemachines mentioned, I love the way this preserves the proportionality between font-sizes — this feels way closer to a magic "it just works" fluidity 👍
One thing I noticed in testing is that the scale in medium viewport sizes feels a little smaller to me than on trunk. I quite like how it feels, but it's noticeable enough that I wonder if it'll potentially read as an unexpected change for sites that already have specific font-sizes selected?
Here's a couple of screenshots to compare:
Trunk vs this PR with the windows set to be half the width of my monitor, with the list view open:
As above, but with the list view closed:
In terms of technical differences in computed font-size, I observed the following with a browser window that is 1265px
wide, with the block inspector open:
- Trunk without list view showing:
104.307px
- This PR without list view showing:
92.0719px
- Trunk with list view showing:
96px
- This PR with list view showing:
71.6366px
With how aggressively the log
scale is applied, I was wondering what the difference in scaling across these mid-range viewports might be if we were to either use a different log base (e.g. Math.log2 instead of a natural logarithm), or if we were to adjust the minimum viewport width?
Overall I love the direction, though, this feels like a good path forward 🎉
lib/block-supports/typography.php
Outdated
$calculated_minimum_font_size = round( $preferred_size['value'] * $default_minimum_font_size_factor, 3 ); | ||
$preferred_font_size_in_px = 'px' === $preferred_size['unit'] ? $preferred_size['value'] : $preferred_size['value'] * 16; | ||
// Logarithmic scale factor: Min font scale that tapers out as the font size increases. | ||
$minimum_font_size_factor = 1 - 0.12 * log( $preferred_font_size_in_px ); |
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.
Please excuse my naivety here: where does the 0.12
value come from, is it just a pleasing value, or is it more significant?
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.
I agree, it feels a bit magic 😄
It affects how quickly the curve will move towards the minimum, that is, how quickly the size factor reaches 0 given increasing font size values.
For example, switching it to 1 - 0.13 * log( $preferred_font_size_in_px );
results in the following curve:
Compare it with 0.12
Thanks for the help with this @andrewserong and @tellthemachines Great suggestions. I play around with it during the week and hopefully arrive at a better balance.
🤔 This is something I'm also concerned about. I'm not sure I can get the algorithm to match its linear counterpart 1:1, but we can try to tweak it to get it close. |
$default_scale_factor = 1; | ||
$has_min_font_size = isset( $fluid_settings['minFontSize'] ) && ! empty( gutenberg_get_typography_value_and_unit( $fluid_settings['minFontSize'] ) ); | ||
$default_minimum_font_size_limit = $has_min_font_size ? $fluid_settings['minFontSize'] : '14px'; | ||
$default_maximum_viewport_width = '1600px'; |
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.
This may be more useful referencing the layout.wideSize value from theme.json, so that fonts stops growing when wide width elements are maxed out—as the wideSize
size is used as the page width in many cases.
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.
Notice how the layout keeps shifting (as the fontSize is still growing), even when the max size is reached:
CleanShot.2023-04-13.at.15.37.49.mp4
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 agree it's definitely something we can and probably should do. It was slated for the original incarnation of fluid font sizes, but I think it fell off radar since we wanted to test the fallback value. From memory it tested pretty well.
I think a manageable approach would be to throw up a new PR using this PR as the base.
👍
I tried out base 2 log and I think it does match more closely what we have, thanks for the suggestion @andrewserong
"settings": {
"layout": {
"contentSize": "840px",
"wideSize": "1100px"
}
} |
…ceiling on the minimum font size
…mum font scale that tapers out as the font size increases. The calculation is only performed where there no min font size is passed to the fluid font size methods. The min font scale factor is constrained by min and max values. Tests are updated to reflect the new clamp values. Docs are updated to reflect API change in JS function (removing minimumFontSizeFactor arg)
8c5b1be
to
f5630e1
Compare
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.
This is feeling quite nice to me with log2
.
Referencing my earlier observations looking at a block set to 128px
and in a browser window that is 1265px wide, with the block inspector open:
- Trunk without list view showing: 104.307px
- Earlier in this PR (log) without list view showing: 92.0719px
- This PR (log2) without list view showing: 95.66px
- Trunk with list view showing: 96px
- This PR (log) with list view showing: 71.6366px
- This PR (log2) with list view showing: 77.2325px
So log2
is looking really pretty close to ideal to me. At iPhone XR size in Chrome, the font-size winds up being 65.735px
which looks about right to me:
This PR does still differ from trunk of course, but then that is the intention. I think it's a cool idea to try out tying the max viewport width to the layout wide size, but again I wonder if that could be an unexpected change for some themes? E.g. if wide size is being used for cover or image blocks to be slightly larger than content size in a theme that has a narrower post content area, but might have larger text in headers and footers. I think I'd probably lean toward seeing if we can look at customising the max viewport width in follow-ups if we can?
Also very curious to hear what designers / theme builders might think of this change — it's looking close to me!
Definitely a worthy follow-up. Thanks for testing @andrewserong ! |
This is a great one, thanks so much for staying on top of this. Once again I can think of few better than @beafialho or @henriqueiamarino or @iamtakashi to test this. If they are available, an easy way to test would be through http://gutenberg.run/?pull=49707 🙏 |
Thank you for your work! I think this works well for most cases. For really huge sizes, in case someone wishes to build something like the example below, it doesn't scale well. But I don't think this is a blocker, as cases like this are exceptions. huge.mp4word.mp4 |
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.
I count two solid sanity checks with potential tweaks or followups. For that reason it can get a green star for the behavior. I'll trust others to double check the code. Should we land this one and tweak the huge headings separately, or amend this one? Thanks for all the work!
Thanks so much for the speedy reviews everyone!!! 🍺 I'll get the following PR into shape as well: |
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.
Great to see folks are liking this change! Just giving this a ✅ from the code side of things, the changes look good to me! ✨
Good point. This is something I'll keep in mind while testing #49815. If folks can reign this sort of behaviour in by adjusting layout values in the editor, even if it is an edge case, I think we'll have won the battle 😆 |
Totally. And I suppose there's also the option of re-introducing a ceiling on the minimum font size as in #49247 if we need to, which could also in theory be a configurable value in |
Definitely!!! Also, do folks think this change is worth a make.wordpress.org shoutout given that it changes behaviour slightly? |
Yes, it'd make a good dev note update. Not sure how urgent it would be for a post, though — perhaps a general fluid typography dev note before 6.3 where we'd include each of the updates if we wind up making further tweaks, too? |
@bph for consideration in future "What's new for developers?" monthly updates on the dev blog! |
* Fluid Typography: Try adding a ceiling to the calculated minimum font size * Add test to ensure ceiling is skipped when min font size is provided * In this commit, we use a logarithmic scale factor to calculate a minimum font scale that tapers out as the font size increases. The calculation is only performed where there no min font size is passed to the fluid font size methods. The min font scale factor is constrained by min and max values. Tests are updated to reflect the new clamp values. Docs are updated to reflect API change in JS function (removing minimumFontSizeFactor arg) * Fix lint and JS tests * EL LINTO DEL DIABLO! * Based on #49707 Testing out using `settings.layout.wideSize` as the maximum viewport width Updating tests to come * Updating tests and generally cleaning up after a bizarre rebase session. * This commit: - elaborates on the purpose of layout.wideSize in the theme.json schema --------- Co-authored-by: Andrew Serong <[email protected]>
What if I want to keep the linear factor? It works with a theme design and has the advantage of keeping the same text breaking on different viewport widths. For edge cases I use additional styling, if necessary. Is there an additional option for the factor? Currently I have to disable fluid font size and use the previous CSS |
Thanks for the comment @strarsis
No there's currently not an option to disable the log scale factor. Note that it only affects fonts that have no defined minimum font size. For example font sizes set manually in the editor, or preset font sizes with no "min" - like "Smallish" below. "settings": {
....
"typography": {
"fluid": true,
"fontSizes": [
{
"size": "1rem",
"slug": "smallish",
"name": "Smallish"
},
{
"size": "2rem",
"fluid": {
"min": "2rem",
"max": "2.5rem"
},
"slug": "mediumish",
"name": "Mediumish"
},
]
} I guess it would be possible to allow an override to allow for scale factors, but rather than adding another property or flag, we could think about exposing a current default setting. I haven't tested it much, but perhaps if there's a theme-defined default min font size factor, or min font size then we skip the log stuff. 🤔 I'm not sure. |
🤔 But exactly that was the case for the theme: The font-sizes all have a |
I tested with theme.json presets with a min value. I bypassed the changes in this PR and logged out to make sure, and can't see that fonts with a defined min size are even making into the condition block. Would you mind posting a few of the affected presets here so I could test in a theme.json file? Also, which version of Gutenberg and WordPress are you using? Thanks! |
Well, that probably is an explanation: I uninstalled the Gutenberg plugin and use the WordPress core Gutenberg functionality instead, as now WordPress core supports all necessary FSE and other features. |
Aha! I believe that WordPress 6.3 and above should reflect the current behavior in Gutenberg. 6.4, which will be released in a few weeks, for sure 😄 |
Tracking issue:
This is an experimental PR that serves as an alternative to #49247, whose commits serve as a base (thanks @andrewserong !)
Maybe resolves #49162
What?
This PR introduces a logarithmic scale factor to calculate a minimum font scale that tapers out as the font size increases.
The calculation is only performed where there no min font size is passed to the fluid font size methods.
The min font scale factor is constrained by min and max values.
There is currently no "ceiling" for minimum font sizes. Rather the scale factor will grow smaller, down to 0.25, as the font size increases.
Why?
The calculation that derives minimum font sizes from font sizes is linear, which means that very large font sizes won't be scaled down to a readable level.
Testing Instructions
I've tested this using the 2023 theme.
In a new post, insert the following example block code. Play around with sizes and values.
Example block code
Run the tests!!
Screenshots or screencast
2023-04-14.12.57.30.mp4