Disable font size picker when fit text is on. #72492
Conversation
jorgefilipecosta
requested review from
ellatrix and
tellthemachines
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jorgefilipecosta
added
[Type] Enhancement
|
Size Change: +59 B (0%) Total Size: 2.17 MB
ℹ️ View Unchanged
|
| settings, | ||
| panelId, | ||
| defaultControls = DEFAULT_CONTROLS, | ||
| fitText = false, |
There was a problem hiding this comment.
An alternative solution to avoid passing a new prop here, would be to on the typography hook packages/block-editor/src/hooks/typography.js change the settings passed when fit text is on. But will add more code without any specific advantage. I'm happy to change if someone prefers the other approach.
There was a problem hiding this comment.
My only hesitation here is what to do about the Site Editor screens: ScreenBlock and ScreenTypographyElement. Will we support fit-text for site-wide elements? What about when setting typography styles for all Paragraphs?
That said, for the purpose of releasing this in 6.9, I'm good.
There was a problem hiding this comment.
My only hesitation here is what to do about the Site Editor screens: ScreenBlock and ScreenTypographyElement. Will we support fit-text for site-wide elements?
There are no plans to support fit-text in global styles, I don't think it makes sense to globally target elements with fit text from global styles (it is not even a css property so technically it would be complex). I expect fit text to be enabled per block when needed.
This component is also used by global styles but the property has a default so it should have no impact for the global styles use case.
| settings, | ||
| panelId, | ||
| defaultControls = DEFAULT_CONTROLS, | ||
| fitText = false, |
There was a problem hiding this comment.
My only hesitation here is what to do about the Site Editor screens: ScreenBlock and ScreenTypographyElement. Will we support fit-text for site-wide elements? What about when setting typography styles for all Paragraphs?
That said, for the purpose of releasing this in 6.9, I'm good.
github-project-automation
bot
moved this to 🔎 Needs Review
in WordPress 6.9 Editor Tasks
|
Flaky tests detected in 72fec2e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18655870133
|
|
@jorgefilipecosta: per #72219 (comment), what if we just disabled the control? How does it feel in practice? |
|
Hi @mcsf, unfortunately font size picker did not had a disabled prop, I added a very simple disable prop (it can benefit from some enhancements) but I think it works ok for now, combined with a help text could be a good solution. I added a commit with this approach. |
mcsf
left a comment
mcsf
left a comment
There was a problem hiding this comment.
@jorgefilipecosta: thanks for trying the disabled route. Currently:
- The visual representation of that state is very poor. Only the toggle button between size presets and custom size changes, as well as the colour of the size unit ("px").
- The size presets still change the cursor to pointer.
|
|
||
| export type FontSizePickerSelectProps = Pick< | ||
| FontSizePickerProps, | ||
| 'value' | 'size' |
There was a problem hiding this comment.
Did you intend to propagate the prop?
There was a problem hiding this comment.
Exactly the idea was to propagate the disable to the components bellow.
Hi @mcsf, I missed the stage & commit to the propagation of the disabled 😔 I was added, I think things should be much better with the latest commit. |
|
This is what I'm seeing now: 
|
|
It may make sense to divide this PR in two if we agree with its direction (one for the addition of disable in FontSizePicker, a pure component enhancement), another one for for the fit text related change. |
|
Thanks, this looks much better. As far as the UX is concerned, this has my approval. However, I'd like the opinion of someone with more authority on Components before we commit to extending all these surfaces. |
Btw, the last one here is the only one that I think needs improvement, no? |
mcsf
changed the title
mcsf
changed the title
Yes the last one is where disabled state is not very noticeable (the border just gets a little lighter), but it is also not completely wrong as interaction with the component gets correctly disabled and clicking does nothing. |
Do we have a consistent pattern for this? Consulting external resources, I'm not convinced disabling is the right path. There's also some prior art for toggles controlling visibility of other fields in settings:
Although one important caveat is that in those examples, the fields come after the toggle, with an implied connection between the toggle and the availability of those fields. As far as whether it makes sense for |
|
I'd be happy to review PRs if we need The only one that's not straightforward is |
I think this is probably the most salient point here. I'd love us to better integrate fit-text into the font-size controls, but as they stand they are a bit detached from one another, and for this reason alone I'm slightly (but only ever so slightly) leaning towards disabling rather than hiding. |
|
The article shared by @aduth is interesting https://www.smashingmagazine.com/2024/05/hidden-vs-disabled-ux/. To the question "Will a given user ever be able to interact with this element?" given that the user just needs to disable fit text to interact with the element the answerer here is yes, so I guess disabling may be a good option. In this case in practical terms given that fit text is bellow it avoids the jump so it has a slightly better us. |
|
I would like to avoid shipping enhancements such as adding new properties to components beyond the 6.9 Beta1 release if possible. What do you think about the following behavior?
|
That's fair.
I'm not opposed to it. :) |
Thank you for the suggestion @t-hamano I'm trying it out. |
|
Closing in favor of #72533. |
github-project-automation
bot
moved this from 🔎 Needs Review
to ✅ Done
in WordPress 6.9 Editor Tasks
Part of the solution to #72219.
When fit text is on the font size is dynamically computed based on the available space, and changing font size values has no impact at all.
The font size attribute is still kept so if the user disables fit text things go back to how they were before.
This PR hides the font size picker when fit text is on.
Screenshots or screencast
Screen.Recording.2025-10-20.at.13.56.53.mov
Testing