Fluid typography: adjust font size min and max rules #45536
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Enhancement
|
|
|
Size Change: -38 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/fluid-font-size-max-min-rules
branch
from
257dc09
to
f6dcac0
Compare
|
Hi, @ramonjd I just wanted to check the status of this PR. Do you think it will be ready for the next minor release? The WP 6.1.1 RC is scheduled for this Friday. |
ramonjd
requested review from
jasmussen,
andrewserong,
noisysocks and
tellthemachines
Hi @Mamaduka, Thanks for checking in. I cannot honestly say. As it stands, this PR is ready to test, and will need thorough testing to ensure it meets folks' expectations. If it turns out that things are working well, then I see no impediment for the minor release. It all depends on the feedback, and the work needed to implement the feedback. |
ramonjd
force-pushed
the
update/fluid-font-size-max-min-rules
branch
from
70ce728
to
2684da7
Compare
ramonjd
commented
Nov 8, 2022
| @@ -461,7 +461,6 @@ function gutenberg_get_typography_font_size_value( $preset, $should_use_fluid_ty | |||
| $default_maximum_viewport_width = '1600px'; | |||
| $default_minimum_viewport_width = '768px'; | |||
| $default_minimum_font_size_factor = 0.75; | |||
| $default_maximum_font_size_factor = 1.5; | |||
There was a problem hiding this comment.
Removed since we never need to compute the max size. We fallback to the incoming "size" or single/custom value. See issue: #45504
andrewserong
reviewed
Nov 8, 2022
There was a problem hiding this comment.
Thanks for the ping, and working on this @ramonjd! Just gave this a quick skim and manual test, and it seems to be working nicely! Left a couple of tiny nits about comments, but nothing major there 🙂
One thing that might be good to get feedback from designers on is whether the scaling multiplier still reduces font sizes enough across viewport sizes. The effect seems a little subtle to me now when setting quite a large font size for the largest viewport size. Whereas before I think because we had the max value scaling up from the desired font-size, there was a greater variance between the computed min and max sizes. To put it simply: now that the selected font size is set for max instead of the middle value, is 0.75 the right multiplier to use, or should it be a smaller fraction?
This is a good question, and I'm not sure what the right fallback min font size multiplier should be. The motivation behind this PR was to give more meaning to the value a user enters, in other words, so that what they see on the screen has a closer relationship with the value they entered. Given that, how much smaller could we get away with in relation to the min font size multiplier without contradicting this principle? So I agree that feedback from themers/designers is crucial here. For those with the time and inclination to test this particular aspect, the default min font size factor (how we auto-generate min font sizes in the absence of an explicit value) can be tweaked:
There's another PR #42489 in the works that will allow the min font size multiplier to be configurable via theme.json. |
|
Thanks for the update, @ramonjd! Let's ping the @WordPress/block-themers group for more feedback. |
|
Why did we choose |
|
Took this for a spin, works well, and feels totally safe to me as a point release improvement, mainly in that it fixes the main headache around using custom font sizes. I set a custom font size to 36px, and it clamps between 27 and 36.
No strong opinion, and happy to defer. As an addendum: even with 16px as the minimum clampable value, it will always be possible for themes to create presets smaller than this, and users to likewise set custom font sizes at nigh unreadable sizes. I like to think we have an opportunity to educate rather than predicate, for example by showing either clarifying help-text, or a contrast-like warning, in the custom font size control. Not something that I think need block this PR. |
|
Thanks @aristath and @jasmussen
I just tested with Example, when I select Should I bump it to cc @tellthemachines for thoughts |
ramonjd
force-pushed
the
update/fluid-font-size-max-min-rules
branch
from
6e89b81
to
863d9ae
Compare
ramonjd
force-pushed
the
update/fluid-font-size-max-min-rules
branch
from
154e3ba
to
01fc69f
Compare
|
Do folks think this one is good to merge, based on the feedback in the issue? |
ramonjd
added
Backport to WP Minor Release
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Nov 10, 2022
- Where no fluid max values are set (e.g., single or custom font size values), the "size" value will act as the maximum value in a clamp() function. - Do not clamp value if a font size is lower than 14px - Always honor a supplied fluid.min value, the lower bound of 14px notwidthstanding
Thank you @Mamaduka !!! I have prepped the PHP backport patch over here: WordPress/wordpress-develop#3602 |
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Nov 10, 2022
- Where no fluid max values are set (e.g., single or custom font size values), the "size" value will act as the maximum value in a clamp() function. - Do not clamp value if a font size is lower than 14px - Always honor a supplied fluid.min value, the lower bound of 14px notwidthstanding
|
Just checking, is there a 6.1.1 release branch or somewhere we need to cherry pick the JS changes into? I see there are few PRs with the "Backport to WP Minor Release" label. |
|
I will cherry-pick the change on the Gutenberg repo, not a problem :) |
Mamaduka
pushed a commit
that referenced
this pull request
Nov 11, 2022
* Initial commit. No tests. * Update PHP unit tests * Updating JS tests Harmonizing test descriptions across PHP and JS Updated CHANGELOG.md and README.md * Updating global styles JS tests * Update typography props JS units tests to reflect new max value * Aligning comments acrossing PHP and JS and making sure they're clearer. Bumping the lower bound from 14px to 16px Updating tests * Updating global styles tests * Revert "Updating global styles tests" This reverts commit 72d4a38. * Revert "Aligning comments acrossing PHP and JS and making sure they're clearer." This reverts commit 863d9ae.
Mamaduka
pushed a commit
that referenced
this pull request
Nov 11, 2022
* Initial commit. No tests. * Update PHP unit tests * Updating JS tests Harmonizing test descriptions across PHP and JS Updated CHANGELOG.md and README.md * Updating global styles JS tests * Update typography props JS units tests to reflect new max value * Aligning comments acrossing PHP and JS and making sure they're clearer. Bumping the lower bound from 14px to 16px Updating tests * Updating global styles tests * Revert "Updating global styles tests" This reverts commit 72d4a38. * Revert "Aligning comments acrossing PHP and JS and making sure they're clearer." This reverts commit 863d9ae.
Mamaduka
removed
the
Backport to WP Minor Release
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of
Resolves #45504
>16px. This applies to custom values from the editor or single-value theme.json styles. Font sizes below this will not be clamped.fluid.minvalue has been specified, the lower bound rule of>16pxwon't be enforced on this value. Presets with a fluid object therefore, give precedence to theme author's values.fluid.maxbut there isfluid.min, use the incoming"size"value as the max.fluid.minbut there is afluid.max, usesize * min_size_factoras the min. Here we do enforce the lower bound rule of>16px, because Gutenberg is computing the min value. This is consistent with the way we calculate minimum sizes for single or custom values.Testing Instructions
Example theme.json
{ "version": 2, "settings": { "typography": { "fluid": true, "fontSizes": [ { "size": "12px", "slug": "piccolino", "name": "Piccolino" }, { "size": "2rem", "fluid": { "min": ".8rem", "max": "40px" }, "slug": "in-mezzo", "name": "In Mezzo" }, { "size": "2.8rem", "fluid": { "min": "0.3rem" }, "slug": "neutrale", "name": "Neutrale" }, { "size": "4.75rem", "slug": "grandone", "name": "Grandone" }, { "size": "6rem", "fluid": { "max": "7rem" }, "slug": "colossal", "name": "Colossal" } ] } }, "styles": { "typography": { "fontSize": "12px" }, "elements": { "h1": { "typography": { "fontSize": "4rem" } } }, "blocks": { "core/post-date": { "typography": { "fontSize": "16px" } } } } }Add some text blocks to the editor, apply presets and custom sizes, and ensure they:
Please test by playing around with: