Fluid typography: ensure max viewport width is used in the editor #51146
Conversation
…maximum viewport size in relation to calculating fluid fontsize values. Unfortunately this maxViewPortWidth value was not being passed to `getTypographyFontSizeValue()` This commit correct this omission and adds extra unit tests to both the JS and PHP versions.
ramonjd
added
[Type] Bug
|
Size Change: +179 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
…gnature the same, hence the gymnastics here.
Because we support fluid: true and fluid: {...values}, we have to ensure the param object is correctly set so that we can know if fluid typography is enabled or not.
The alternative could be a third argument to the `getTypographyFontSizeValue()` function, e.g., `isFluidEnabled`
| valueFunc: ( preset, settings ) => | ||
| getTypographyFontSizeValue( | ||
| preset, | ||
| !! settings?.typography?.fluid && settings?.layout?.wideSize |
There was a problem hiding this comment.
To avoid backwards compat problems I'm trying to keep the function signature the same, hence the gymnastics here.
Because we support fluid: true and fluid: {...values}, we have to ensure the param object is correctly set so that we can know if fluid typography is enabled or not.
Also, we need to pass settings?.layout?.wideSize if available only when fluid font sizes are enabled. 😵
The alternative could be a third argument to the getTypographyFontSizeValue() function, e.g., isFluidEnabled.
Or add a prop and pass the following second argument to getTypographyFontSizeValue:
{
....settings?.typography,
layoutWideSize: settings?.layout?.wideSize
}
and move the logic there? I think that might be better.
There was a problem hiding this comment.
It is a bit of gymnastics to maintain back compat, but I actually don't mind it how it's written as-is, as it means that the calling code is responsible for ensuring that the spread settings.typography.fluid overrides the maxViewPortWidth set via the layout wideSize, and the getTypographyFontSizeValue doesn't have to work too hard to figure it out.
That said, adding layoutWideSize to TypographySettings could be a clever way to do it, then it'd be fairly straightforward to use layoutWideSize only if maxViewPortWidth doesn't exist.
Whatever you think works best, I don't think I mind either way, personally!
There was a problem hiding this comment.
Not all that familiar with this part of the codebase so take this with a large grain of salt.
What if we extract all of the code in getTypographyFontSizeValue that transforms the settings.typography object into an object that's suitable for passing to getComputedFluidTypographyValue into a new function?
That way valueFunc can set maximumViewPortWidth on a normalised object and not have to deal with the intricacies of parsing the settings.typography.fluid boolean / object.
valueFunc: ( preset, settings ) => {
const fluidTypographySettings = getFluidTypographySettings( settings?.typography );
fluidSettings.maximumViewPortWidth = settings?.layout?.wideSize;
return getComputedFluidTypographyValue( fluidSettings );
},There was a problem hiding this comment.
I don't think what you have is that bad though. This suggestion is for if it's really bugging you 😀
There was a problem hiding this comment.
What if we extract all of the code in getTypographyFontSizeValue that transforms the settings.typography object into an object that's suitable for passing to getComputedFluidTypographyValue into a new function?
yes
yes
YES!
packages/block-editor/src/components/global-styles/typography-utils.js exists for a reason 😄
There was a problem hiding this comment.
I don't think what you have is that bad though. This suggestion is for if it's really bugging you 😀
Thanks for helping @noisysocks and @andrewserong
I might merge as is and mark this suggestion for a follow up, since it might come in handy down the track.
There was a problem hiding this comment.
Nice work @ramonjd, this is testing well for me. I see in #50929 the issue report mentioned thinking that the FSE version looked correct, but the page editor / site frontend version seemed inconsistent. I think the change in this PR is the right way around, as it's rolling out the wideSize max viewport width approach to the site editor 👍
✅ Confirmed the issue on trunk using TT3
✅ Confirmed with this PR applied, the calculated fluid sizes now match between site editor and site frontend
| Before (the font size is smaller in the site editor) | After (the font size is larger in the site editor) |
|---|---|
LGTM! ✨
packages/block-editor/src/components/global-styles/typography-utils.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
| valueFunc: ( preset, settings ) => | ||
| getTypographyFontSizeValue( | ||
| preset, | ||
| !! settings?.typography?.fluid && settings?.layout?.wideSize |
There was a problem hiding this comment.
It is a bit of gymnastics to maintain back compat, but I actually don't mind it how it's written as-is, as it means that the calling code is responsible for ensuring that the spread settings.typography.fluid overrides the maxViewPortWidth set via the layout wideSize, and the getTypographyFontSizeValue doesn't have to work too hard to figure it out.
That said, adding layoutWideSize to TypographySettings could be a clever way to do it, then it'd be fairly straightforward to use layoutWideSize only if maxViewPortWidth doesn't exist.
Whatever you think works best, I don't think I mind either way, personally!
|
This didn't make it into 16.0.0? Edit: Never mind; I see that it did, but it didn't take into account values for content width that are css functions. |
Thanks, I'll take a look. For now I think we should ignore CSS functions in the layout object for the purposes of calculating the linear scale factor. |
Same — it's good to know that there's the use case of themes that use css functions in these values, but I think it's probably best to ignore them for now as it'd be difficult to reliably use them for the calculation. |
|
Alternatively, we could use css calc directly, and not have to deal with php calculations. My understanding that it was a simple enhancement (using log) that created this overhead, and maybe we could reduce a ton of complexity by not doing that. Regardless, I've "solved" my problem by maintaining my own duplicated css overrides and not using the WP calculations at all. |
I believe the calculation was always going to run into trouble with CSS functions, even before the log change. The way the linear factor is calculated hasn't changed. The log calculation only affected the minimum font size. See: Once things have stabilized, there's also the opportunity to introduce some filters that might open the doors to greater flexibility and control for devs/theme authors.
👍 |
|
The linear version is possible in css using calc() and var()s. I have a separate ticket about it 😄 |
|
@ramonjd Thanks for all your work here. In case you are interested, I have a working version of the CSS only equation: :root {
--large: 1600;
--small: 960;
--max: 25;
--min: 10;
--rem: 16;
}
* {
--func--fluid-rem: clamp(
calc(var(--min) * 1px),
calc((var(--min) / var(--rem) * 1rem) + ( ( 100vw - (var(--small) * 1px) ) * ((var(--max) - var(--min)) / (var(--large) - var(--small))) )),
calc(var(--max) * 1px)
);
}
.use-css-func {
--max: 22;
--min: 18;
font-size: var(--func--fluid-rem);
}<div class="use-css-func" style="--max: 50;--min: 20;">Bonus: It takes into account both breakpoints, and is customizable via |
|
Nice, thanks for posting that. It might come in handy down the track ;) |
…rdPress#51146) * WordPress#49815 added `settings.layout.wideSize` as the default value for the maximum viewport size in relation to calculating fluid fontsize values. Unfortunately this maxViewPortWidth value was not being passed to `getTypographyFontSizeValue()` This commit correct this omission and adds extra unit tests to both the JS and PHP versions. * To avoid backwards compat problems I'm trying to keep the function signature the same, hence the gymnastics here. Because we support fluid: true and fluid: {...values}, we have to ensure the param object is correctly set so that we can know if fluid typography is enabled or not. The alternative could be a third argument to the `getTypographyFontSizeValue()` function, e.g., `isFluidEnabled` * My blod is typo * Why chain? Break the chains!
Tracking issue:
Fixes #50929
What?
#49815 added
settings.layout.wideSizeas the default value for the maximum viewport size in relation to calculating fluid font-size values.Unfortunately this
maxViewPortWidthvalue was not being passed togetTypographyFontSizeValue().This commit correct this omission and adds extra unit tests to both the JS and PHP versions.
Props to @WraithKenny for spotting it 🙇
Why?
As described in #50929, the site editor is outputting the wrong value when a
settings.layout.wideSizevalue is set for the theme. This is because it is completely ignored said value.The backend outputs the correct value since it is taking into account
settings.layout.wideSize.How?
Ensuring that
settings.layout.wideSizeis passed togetTypographyFontSizeValue().Testing Instructions
Run tests!
You can test using twentytwentythree theme!
Add some text to the site editor and to a post. Apply a font size, e.g.,
xx-largeSave the post/site.
Now inspect the font-size CSS value while still in the editor.
Compare this value with what you see on the frontend.
The CSS preset value should match in the post editor, site editor and frontend.
Here's the class:
And the preset:
If you'd like to play around with the values, here is some test HTML and JSON.
Here is some example HTML
Here is some example theme.json
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "typography": { "fluid": true, "fontSizes": [ { "fluid": { "min": "0.875rem", "max": "1rem" }, "size": "1rem", "slug": "small" }, { "fluid": { "min": "1rem", "max": "1.125rem" }, "size": "1.125rem", "slug": "medium" }, { "fluid": { "min": "1.75rem", "max": "1.875rem" }, "size": "1.75rem", "slug": "large" }, { "fluid": { "max": "125px", "min": "70px" }, "size": "80px", "slug": "x-large" }, { "fluid": { "max": "7.8125rem", "min": "4.375rem" }, "size": "7.8125rem", "slug": "xx-large" } ] } } }