Skip to content
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.

Sign up for GitHub

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: ensure max viewport width is used in the editor #51146

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Fluid typography: ensure max viewport width is used in the editor #51146

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0cc7625
#49815 added `settings.layout.wideSize` as the default value for the …
ramonjd Jun 1, 2023
2afe357
To avoid backwards compat problems I'm trying to keep the function si…
ramonjd Jun 1, 2023
a247dfd
My blod is typo
ramonjd Jun 1, 2023
dfb7131
Why chain? Break the chains!
ramonjd Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions packages/block-editor/src/components/global-styles/test/typography-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,58 @@ describe( 'typography utils', () => {
'clamp(17.905px, 1.119rem + ((1vw - 3.2px) * 0.789), 28px)',
},

{
message:
'returns clamp value where min and max fluid values defined',
preset: {
size: '80px',
fluid: {
min: '70px',
max: '125px',
},
},
typographySettings: {
fluid: true,
},
expected:
'clamp(70px, 4.375rem + ((1vw - 3.2px) * 4.297), 125px)',
},

{
message:
'should apply maxViewPortWidth as maximum viewport width',
preset: {
size: '80px',
fluid: {
min: '70px',
max: '125px',
},
},
typographySettings: {
fluid: {
maxViewPortWidth: '1100px',
},
},
expected:
'clamp(70px, 4.375rem + ((1vw - 3.2px) * 7.051), 125px)',
},

{
message: 'returns clamp value where max is equal to size',
preset: {
size: '7.8125rem',
fluid: {
min: '4.375rem',
max: '7.8125rem',
},
},
typographySettings: {
fluid: true,
},
expected:
'clamp(4.375rem, 4.375rem + ((1vw - 0.2rem) * 4.298), 7.8125rem)',
},

{
message:
'should return clamp value if min font size is greater than max',
Expand Down
14 changes: 7 additions & 7 deletions packages/block-editor/src/components/global-styles/typography-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,16 @@ import { getComputedFluidTypographyValue } from '../font-sizes/fluid-utils';
* @property {?string|?number} size A default font size.
* @property {string} name A font size name, displayed in the UI.
* @property {string} slug A font size slug
* @property {boolean|FluidPreset|undefined} fluid A font size slug
* @property {boolean|FluidPreset|undefined} fluid Specifics the minimum and maximum font size value of a fluid font size.
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
*/

/**
* @typedef {Object} TypographySettings
* @property {?string|?number} size A default font size.
* @property {?string} minViewPortWidth Minimum viewport size from which type will have fluidity. Optional if size is specified.
* @property {?string} maxViewPortWidth Maximum size up to which type will have fluidity. Optional if size is specified.
* @property {?number} scaleFactor A scale factor to determine how fast a font scales within boundaries. Optional.
* @property {?number} minFontSizeFactor How much to scale defaultFontSize by to derive minimumFontSize. Optional.
* @property {?string} minFontSize The smallest a calculated font size may be. Optional.
* @property {?string} minViewPortWidth Minimum viewport size from which type will have fluidity. Optional if size is specified.
* @property {?string} maxViewPortWidth Maximum size up to which type will have fluidity. Optional if size is specified.
* @property {?number} scaleFactor A scale factor to determine how fast a font scales within boundaries. Optional.
* @property {?number} minFontSizeFactor How much to scale defaultFontSize by to derive minimumFontSize. Optional.
* @property {?string} minFontSize The smallest a calculated font size may be. Optional.
*/

/**
Expand Down Expand Up @@ -77,6 +76,7 @@ export function getTypographyFontSizeValue( preset, typographySettings ) {
maximumFontSize: preset?.fluid?.max,
fontSize: defaultSize,
minimumFontSizeLimit: fluidTypographySettings?.minFontSize,
maximumViewPortWidth: fluidTypographySettings?.maxViewPortWidth,
} );

if ( !! fluidFontSizeValue ) {
Expand Down
15 changes: 14 additions & 1 deletion packages/block-editor/src/components/global-styles/use-global-styles-output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,9 +396,22 @@ export function getStylesDeclarations(
* Values that already have a "clamp()" function will not pass the test,
* and therefore the original $value will be returned.
*/
const typographySettings =
!! tree?.settings?.typography?.fluid &&
tree?.settings?.layout?.wideSize
? {
fluid: {
maxViewPortWidth:
tree?.settings?.layout?.wideSize,
...tree?.settings?.typography?.fluid,
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
},
}
: {
fluid: tree?.settings?.typography?.fluid,
};
ruleValue = getTypographyFontSizeValue(
{ size: ruleValue },
tree?.settings?.typography
typographySettings
);
}

Expand Down
16 changes: 14 additions & 2 deletions packages/block-editor/src/components/global-styles/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,20 @@ export const PRESET_METADATA = [
},
{
path: [ 'typography', 'fontSizes' ],
valueFunc: ( preset, { typography: typographySettings } ) =>
getTypographyFontSizeValue( preset, typographySettings ),
valueFunc: ( preset, settings ) =>
getTypographyFontSizeValue(
preset,
!! settings?.typography?.fluid && settings?.layout?.wideSize
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think what you have is that bad though. This suggestion is for if it's really bugging you 😀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? {
fluid: {
maxViewPortWidth: settings?.layout?.wideSize,
...settings?.typography?.fluid,
},
}
: {
fluid: settings?.typography?.fluid,
}
),
valueKey: 'size',
cssVarInfix: 'font-size',
classes: [ { classSuffix: 'font-size', propertyName: 'font-size' } ],
Expand Down
24 changes: 24 additions & 0 deletions phpunit/block-supports/typography-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,30 @@ public function data_generate_font_size_preset_fixtures() {
'expected_output' => 'clamp(17.905px, 1.119rem + ((1vw - 3.2px) * 0.789), 28px)',
),

'returns clamp value where min and max fluid values defined' => array(
'font_size' => array(
'size' => '80px',
'fluid' => array(
'min' => '70px',
'max' => '125px',
),
),
'should_use_fluid_typography' => true,
'expected_output' => 'clamp(70px, 4.375rem + ((1vw - 3.2px) * 4.297), 125px)',
),

'returns clamp value where max is equal to size' => array(
'font_size' => array(
'size' => '7.8125rem',
'fluid' => array(
'min' => '4.375rem',
'max' => '7.8125rem',
),
),
'should_use_fluid_typography' => true,
'expected_output' => 'clamp(4.375rem, 4.375rem + ((1vw - 0.2rem) * 4.298), 7.8125rem)',
),

'returns clamp value if min font size is greater than max' => array(
'font_size' => array(
'size' => '3rem',
Expand Down