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.

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 global styles preset fluid sizes are calculated correctly #44791

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
Original file line number Diff line number Diff line change
Expand Up @@ -1047,8 +1047,15 @@ protected static function compute_style_properties( $styles, $settings = array()
continue;
}

// Calculate fluid typography rules where available.
// Calculates fluid typography rules where available.
if ( 'font-size' === $css_property ) {
/*
* gutenberg_get_typography_font_size_value() will check
* if fluid typography has been activated and also
* whether the incoming value can be converted to a fluid value.
* Values that already have a "clamp()" function will not pass the test,
* and therefore the original $value will be returned.
*/
$value = gutenberg_get_typography_font_size_value( array( 'size' => $value ) );
}

Expand Down
8 changes: 7 additions & 1 deletion packages/components/src/font-size-picker/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,13 @@ const UnforwardedFontSizePicker = (
),
[ shouldUseSelectControl, fontSizes, disableCustomFontSizes ]
);
const selectedOption = getSelectedOption( fontSizes, value );
const selectedOption = getSelectedOption(
fontSizes,
value,
shouldUseSelectControl,
disableCustomFontSizes
);

const isCustomValue = selectedOption.slug === CUSTOM_FONT_SIZE;
const [ showCustomValueControl, setShowCustomValueControl ] = useState(
! disableCustomFontSizes && isCustomValue
Expand Down
15 changes: 10 additions & 5 deletions packages/components/src/font-size-picker/test/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,33 +120,38 @@ describe( 'getToggleGroupOptions', () => {
).toEqual( [
{
key: '1',
value: '1',
label: 'S',
name: '1',
size: '1',
value: '1',
},
{
key: '2',
value: '2',
label: 'M',
name: '2',
size: '2',
value: '2',
},
{
key: '3',
value: '3',
label: 'L',
name: '3',
size: '3',
value: '3',
},
{
key: '4',
value: '4',
label: 'XL',
name: '4',
size: '4',
value: '4',
},
{
key: '5',
value: '5',
label: 'XXL',
name: 'XXL',
value: '5',
size: '5',
},
] );
} );
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/font-size-picker/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,5 @@ export type FontSizeToggleGroupOption = {
value: number | string;
label: string;
name: string;
size?: number | string;
};
27 changes: 22 additions & 5 deletions packages/components/src/font-size-picker/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ function getSelectOptions(
];
return options.map( ( { slug, name, size } ) => ( {
key: slug,
name,
name: name || slug,
label: name || slug,
slug,
size,
__experimentalHint:
size && isSimpleCssValue( size ) && parseFloat( String( size ) ),
Expand All @@ -134,6 +136,7 @@ export function getToggleGroupOptions(
return {
key: slug,
value: size,
size,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need both size and value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Long story :)

It's the way ToggleGroupControl and ToggleGroupControlOption work.

The latter uses a value prop for the font size, the former size. See:

size?: 'default' | '__unstable-large';

We need both so the current size is displayed correctly in the options and the ToggleGroupControl header.

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 I wrote above is complete rubbish. Ignore me.

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 I wrote above is complete rubbish. Ignore me.

Actually, I was right and I didn't know it. The size has to stay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I was right and I didn't know it. The size has to stay.

Actually I'm wrong and I didn't know it. It's the headerHint I need to change. Ignore everything.

label: labelAliases[ index ],
name: name || labelAliases[ index ],
};
Expand All @@ -142,13 +145,27 @@ export function getToggleGroupOptions(

export function getSelectedOption(
fontSizes: FontSize[],
value: FontSizePickerProps[ 'value' ]
value: FontSizePickerProps[ 'value' ],
useSelectControl: boolean,
disableCustomFontSizes: boolean = false
): FontSizeOption {
Copy link
Member

Choose a reason for hiding this comment

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

The return type here is FontSizeOption but your changes make it so that the return type is FontSizeSelectOption | FontSizeToggleGroupOption. This should be a type error but I guess it's not because of the // @ts-ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point. That was an oversight. The // @ts-ignore is for something else entirely. I'll update.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other PR coz I just merged 😄

Copy link
Member Author

@ramonjd ramonjd Oct 10, 2022

Choose a reason for hiding this comment

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

Here's what I need to do to make TS shut up

diff --git a/packages/components/src/font-size-picker/types.ts b/packages/components/src/font-size-picker/types.ts
index ec193f51bf..1df66bb922 100644
--- a/packages/components/src/font-size-picker/types.ts
+++ b/packages/components/src/font-size-picker/types.ts
@@ -92,6 +92,7 @@ export type FontSizeSelectOption = Pick< FontSizeOption, 'size' > & {
 
 export type FontSizeToggleGroupOption = {
 	key: string;
+	slug: string;
 	value: number | string;
 	label: string;
 	name: string;
diff --git a/packages/components/src/font-size-picker/utils.ts b/packages/components/src/font-size-picker/utils.ts
index 02427e0419..b59c3609a2 100644
--- a/packages/components/src/font-size-picker/utils.ts
+++ b/packages/components/src/font-size-picker/utils.ts
@@ -135,6 +135,7 @@ export function getToggleGroupOptions(
 	return optionsArray.map( ( { slug, size, name }, index ) => {
 		return {
 			key: slug,
+			slug,
 			value: size,
 			size,
 			label: labelAliases[ index ],
@@ -148,7 +149,7 @@ export function getSelectedOption(
 	value: FontSizePickerProps[ 'value' ],
 	useSelectControl: boolean,
 	disableCustomFontSizes: boolean = false
-): FontSizeOption {
+): FontSizeOption | FontSizeToggleGroupOption {
 	if ( ! value ) {
 		return DEFAULT_FONT_SIZE_OPTION;
 	}

if ( ! value ) {
return DEFAULT_FONT_SIZE_OPTION;
}
return (
fontSizes.find( ( font ) => font.size === value ) ||
CUSTOM_FONT_SIZE_OPTION

const fontSizeOptions = getFontSizeOptions(
useSelectControl,
fontSizes,
disableCustomFontSizes
);

const selectedOption = fontSizeOptions
? // @TODO Array.find() triggers error on array types unions. It's a bug. See: https://github.com/microsoft/TypeScript/issues/44373.
// @ts-ignore
fontSizeOptions.find(
( option: FontSizeSelectOption ) => option.size === value
) // @ts-ignore
: null;

return selectedOption || CUSTOM_FONT_SIZE_OPTION;
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { getSupportedGlobalStylesPanels, useSetting, useStyle } from './hooks';
import { getTypographyFontSizeValue } from './typography-utils';

export function useHasTypographyPanel( name ) {
const hasFontFamily = useHasFontFamilyControl( name );
Expand Down Expand Up @@ -151,7 +152,19 @@ export default function TypographyPanel( { name, element, headingLevel } ) {
} else if ( element && element !== 'text' ) {
prefix = `elements.${ element }.`;
}
const [ fluidTypography ] = useSetting( 'typography.fluid', name );
const [ fontSizes ] = useSetting( 'typography.fontSizes', name );

// Convert static font size values to fluid font sizes if fluidTypography is activated.
const fontSizesWithFluidValues = fontSizes.map( ( font ) => {
if ( !! fluidTypography ) {
font.size = getTypographyFontSizeValue( font, {
fluid: fluidTypography,
} );
}
return font;
} );

const disableCustomFontSizes = ! useSetting(
'typography.customFontSize',
name
Expand Down Expand Up @@ -240,7 +253,7 @@ export default function TypographyPanel( { name, element, headingLevel } ) {
<FontSizePicker
value={ fontSize }
onChange={ setFontSize }
fontSizes={ fontSizes }
fontSizes={ fontSizesWithFluidValues }
disableCustomFontSizes={ disableCustomFontSizes }
withReset={ false }
size="__unstable-large"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,13 @@ export function getStylesDeclarations(

// Calculate fluid typography rules where available.
if ( cssProperty === 'font-size' ) {
/*
* getTypographyFontSizeValue() will check
* if fluid typography has been activated and also
* whether the incoming value can be converted to a fluid value.
* Values that already have a "clamp()" function will not pass the test,
* and therefore the original $value will be returned.
*/
ruleValue = getTypographyFontSizeValue(
{ size: ruleValue },
tree?.settings?.typography
Expand Down