-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
394d74c
4d4b550
2e06a67
9d5cfe7
f5ecb56
2ceb581
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ) ), | ||
|
@@ -134,6 +136,7 @@ export function getToggleGroupOptions( | |
return { | ||
key: slug, | ||
value: size, | ||
size, | ||
label: labelAliases[ index ], | ||
name: name || labelAliases[ index ], | ||
}; | ||
|
@@ -142,13 +145,27 @@ export function getToggleGroupOptions( | |
|
||
export function getSelectedOption( | ||
fontSizes: FontSize[], | ||
value: FontSizePickerProps[ 'value' ] | ||
value: FontSizePickerProps[ 'value' ], | ||
useSelectControl: boolean, | ||
disableCustomFontSizes: boolean = false | ||
): FontSizeOption { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type here is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good point. That was an oversight. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other PR coz I just merged 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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.
Why do we need both
size
andvalue
?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.
Long story :)
It's the way
ToggleGroupControl
andToggleGroupControlOption
work.The latter uses a
value
prop for the font size, the formersize
. See:gutenberg/packages/components/src/toggle-group-control/types.ts
Line 126 in d66796f
We need both so the current size is displayed correctly in the options and the
ToggleGroupControl
header.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.
What I wrote above is complete rubbish. Ignore me.
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.
Actually, I was right and I didn't know it. The
size
has to stay.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.
Actually I'm wrong and I didn't know it. It's the
headerHint
I need to change. Ignore everything.