-
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
Fluid Typography: ensure global styles preset fluid sizes are calculated correctly #44791
Conversation
Size Change: +96 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
213619d
to
9b70e4d
Compare
9b70e4d
to
4d4b550
Compare
…o use getFontSizeOptions(), but I guess we'll have to refactor.
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 working great for me!
I can now see the correct fluid font size based on the available preset parameters. For example, in TT3 I can see when I set all H1s to the XXL font size, the clamp calculation with the same values from theme.json is used:
I can also confirm that the XXL label is shown correctly in the Global Styles UI:
6eea350
to
f5ecb56
Compare
You'll need a |
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 the fix here @ramonjd, I think this is a good pragmatic fix for the problem at hand — it solves the problem by ensuring that the clampified values are saved, instead of the raw non-clampified values, which ensures that when fluid
is set to true
, the fluid font-sizes are in use.
However, I'm also wondering if the behaviour here reveals something slightly unexpected in how preset font size values are saved in global styles versus in the post editor. In the post editor, when a preset is selected, we use the fontSize
attribute to store the slug of the preset. But in global styles the real value of the preset is used instead of storing a reference to the CSS variable, which creates an interesting problem when we try toggling the fluid
setting. I'll try to walk through:
With fluid
set to false
, set the H1 element to use xx-large
based on the sample theme.json
settings. Then, update the theme.json
file to set fluid
to true
, and reload the site editor. Notice that global styles now thinks that a custom value is in use, because the value differs to the clampified value:
Fluid set to false |
Fluid set to true (existing value looks like a custom value now) |
---|---|
If we go from the other direction (set a fluid typography preset and then toggle off the fluid
value), then when we reload, the font size picker displays an empty custom value field (because the current clampified value cannot be displayed in the UnitControl). So it appears empty:
The last problem is that the clampified values don't appear to work with the value label in the select view. Note that the grey values next to the slugs aren't visible in the fluid view in global styles, however they are visible in the post editor view:
Global styles | Post editor |
---|---|
Now, all that said — I think some of these things are things we can potentially look into beyond 6.1. Because user values for global styles are saved at the per-theme level, and it's unlikely that someone will be toggling fluid
, in practice, I think it's doubtful that folks will really run into the issue of using real values all that frequently.
So, please excuse the long rambling explanation above as I think through the trade-offs! For expedience, and to resolve the bug at hand, I think this is a good fix to go with (and it addresses the bug at hand). For 6.2, though, it might be nice to for us to dig in a little further to how global styles saves font sizes, and see if we can add some better flexibility / referencing the CSS variables, along with exposing UI controls surrounding fluid typography.
What do you think?
Thanks for testing @noisysocks and @andrewserong
Yeah it appears inconsistent. I wonder if we could categorize it, as you already mention, as an idiosyncrasy of the site editor/presets. There is a slightly weak argument that says, well, if you're toggling fluidity on and off for individual font sizes, any presently saved value will be custom since there's no equivalent in the presets collection. But it would still be neat to have a value in the UI. Perhaps we could at least have a fallback placeholder value.
I agree. And also with your observation that, for now, there won't be much toggling of individual font sizes so maybe it's moot for 6.1.
I'm thinking that resolving this might be doable. I'll have a play and get back to you. |
It looks like this has always been true for any non-simple CSS value:
We pass a So, even in the days before fluid typography we didn't know how to deal with Maybe in the future we should have a more meaningful hint depending on the type of value: Long story short: I think we can punt it. |
Sounds good 👍 |
@@ -134,6 +136,7 @@ export function getToggleGroupOptions( | |||
return { | |||
key: slug, | |||
value: size, | |||
size, |
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
and value
?
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
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.
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.
What I wrote above is complete rubbish. Ignore me.
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 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.
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 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
.
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.
Oh good point. That was an oversight. The // @ts-ignore
is for something else entirely. I'll update.
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.
On the other PR coz I just merged 😄
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.
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;
}
…ted correctly (#44791) * Forked #44761 * Ensuring the font size picker select box shows the right labels * update comment. Typescript has beaten me. It's much more convenient to use getFontSizeOptions(), but I guess we'll have to refactor. * Adding comment about Typescript bug (YAY, it wasn't me being dumb with TS for once) * Added tests yo * Changeo loggo
1. Clean up type changes in #44791 2. Make TS linter be quiet
@ramonjd @noisysocks does this need to be backported to 6.1? |
Thanks @ndiego This PR has already been merged into #44761, which needs to be backported. Therefore this PR can be safely ignored for the purposes of backporting. 🍺 |
…re not calculated for custom values (#44761) * Fluid Typography: Fix bug where fluid clamp rules were not calculated for custom global styles values * Add inline comments * Add tests for JS changes * Fluid Typography: ensure global styles preset fluid sizes are calculated correctly (#44791) * Forked #44761 * Ensuring the font size picker select box shows the right labels * update comment. Typescript has beaten me. It's much more convenient to use getFontSizeOptions(), but I guess we'll have to refactor. * Adding comment about Typescript bug (YAY, it wasn't me being dumb with TS for once) * Added tests yo * Changeo loggo * Create a new FontSizeSelectOption return type for getSelectedOption to: 1. Clean up type changes in #44791 2. Make TS linter be quiet * Revert accidental changes to emptytheme * Revert type changes and other calamities * Remove additional size value from getToggleGroupOptions test as I believe it is no longer expected Co-authored-by: Ramon <[email protected]> Co-authored-by: ramonjd <[email protected]>
…re not calculated for custom values (#44761) * Fluid Typography: Fix bug where fluid clamp rules were not calculated for custom global styles values * Add inline comments * Add tests for JS changes * Fluid Typography: ensure global styles preset fluid sizes are calculated correctly (#44791) * Forked #44761 * Ensuring the font size picker select box shows the right labels * update comment. Typescript has beaten me. It's much more convenient to use getFontSizeOptions(), but I guess we'll have to refactor. * Adding comment about Typescript bug (YAY, it wasn't me being dumb with TS for once) * Added tests yo * Changeo loggo * Create a new FontSizeSelectOption return type for getSelectedOption to: 1. Clean up type changes in #44791 2. Make TS linter be quiet * Revert accidental changes to emptytheme * Revert type changes and other calamities * Remove additional size value from getToggleGroupOptions test as I believe it is no longer expected Co-authored-by: Ramon <[email protected]> Co-authored-by: ramonjd <[email protected]>
* Fluid typography: convert server-side block support values (#44762) * For serverside rendered typography block support styles, covert font size values to fluid values in the inline styles if fluid typography is activated. Added unit tests * Add fluid font size to Nav Link * Add fluid typography to Search block * Fix fluid typography for Submenu block with open on click * Fix fluid typography for Page List block * Remove unnecessary parameter * Call esc_attr only once on the whole typography string Co-authored-by: tellthemachines <[email protected]> * Fluid Typography: Fix bug in global styles where fluid clamp rules were not calculated for custom values (#44761) * Fluid Typography: Fix bug where fluid clamp rules were not calculated for custom global styles values * Add inline comments * Add tests for JS changes * Fluid Typography: ensure global styles preset fluid sizes are calculated correctly (#44791) * Forked #44761 * Ensuring the font size picker select box shows the right labels * update comment. Typescript has beaten me. It's much more convenient to use getFontSizeOptions(), but I guess we'll have to refactor. * Adding comment about Typescript bug (YAY, it wasn't me being dumb with TS for once) * Added tests yo * Changeo loggo * Create a new FontSizeSelectOption return type for getSelectedOption to: 1. Clean up type changes in #44791 2. Make TS linter be quiet * Revert accidental changes to emptytheme * Revert type changes and other calamities * Remove additional size value from getToggleGroupOptions test as I believe it is no longer expected Co-authored-by: Ramon <[email protected]> Co-authored-by: ramonjd <[email protected]> * Fluid typography: convert font size inline style attributes to fluid values (#44764) * This commit ensures that custom font size values that appear in the style attribute of block content are converted to fluid typography (if activated) * Adding comments * Bail early if we don't find a custom font size * Adding tests yo * Updating PHP doc to describe incoming type of $raw_value (string|number) * Make custom font sizes appear fluid in the block editor when fluid typography is enabled (#44765) * Make custom font sizes appear fluid in the block editor when fluid typography is enabled * Add tests for fluid utils * update description * You shall not pass with a number, well, yes, but we'll coerce it to `px` and the tests shall pass nonetheless!!! Co-authored-by: Ben Dwyer <[email protected]> Co-authored-by: ramonjd <[email protected]> * Fluid typography: covert font size number values to pixels (#44807) * Converts incoming raw font size values to value + pixel to remain consistent with the fontsizepicker component. * Updating comments / docs * Fluid typography: ensure fontsizes are strings or integers (#44847) * Updating PHP doc to describe incoming type of $raw_value (string|int) This PR also harmonizes the JS checks And review comments from #44807 (review) These changes have already been backported to Core in WordPress/wordpress-develop#3428 * Update changelog Add extra test for floats Add i18n domain * Font sizes can be string|int|float Updated tests and JSDoc type * Expand tests for gutenberg_get_typography_value_and_unit Fix typo in CHANGELOG.md * Initial commit (#44852) - ensure that we convert fluid font sizes to fluid values in the editor for search block block supports - we do this by passing the getTypographyClassesAndStyles hook a flag to tell it whether to convert Updated CHANGELOG.md Added tests Co-authored-by: tellthemachines <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Robert Anderson <[email protected]> Co-authored-by: Ben Dwyer <[email protected]>
@ramonjd, I'm seeing the 'undefined' label again with the XXL font size on WP 6.1 RC1 and Gutenberg trunk. Happy to create another issue. |
I think I have a fix for the 'undefined' issue described above. It looks like it's only a problem when the theme does not provide |
Resolves #44752
What
This PR is from #44761 to fix #44752, and fixes another bug that forces preset values selected in the site editor are calculated to use any available preset fluid parameters.
It also ensure that the control UI for the font size picker works as intended:
2022-10-08.20.38.00.mp4
2022-10-08.20.38.54.mp4
How
By pre-calculating fluid values before building the fontsizepicker controls so that the clamp() value is saved.
Custom font size values will be calculated by getStyleDeclarations
Testing Instructions
Try with TT3 or the example JSON:
Example theme.json