Add vw and vh units to the custom font size picker #60607
Conversation
|
@ramonjd Hi, I am pinging you early because I think I am missing something. I am trying to add the vw and vh units to the font size picker, and I don't think it can be as easy as this? I can't help thinking that, if it only required this code change, why did we not add it earlier? There must be a rabbit hole somewhere :) |
|
Size Change: +2.19 kB (+0.13%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Thanks for the ping @carolinan
How many times have I thought this too 😄 ? The PR tests well in the editor. I can't think of any technical reason why we couldn't support these units. Not to say there aren't rabbit holes, yet from a code perspective I can't see any obvious ones. The only issue that occurs to me right now is that fluid typography doesn't yet support So a user might be left wondering why some units are converted to clamp values and others not. I suppose one day this could be communicated via the UI somehow, or integrated into any controls we might consider, e.g., Pinging @jasmussen for a design perspective. |
|
I suppose we could ask the community if this would be useful without clamp(). I'll ask in the issue. |
|
The motivation for fluid typography is one of "it just works", and both vh and vw units are fluid in their own way. To that end I'd think this could be as simple as just allowing those units as Carolina suggests. There's still a very separate question around how the initial theme.json font size array can be built using the editor, and since theme.json allows a font size preset to be fluid or not, that's something you need to be able to build using global styles. That's mostly a UI design question, but it might eventually overlap with some of aspects of this conversation. |
carolinan
changed the title
|
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. |
|
Flaky tests detected in 2f45c69. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9202165130
|
|
Just to first confirm that we're making the intended changes — do we want to expand the unit types that are available in the Typography panel only? Or expand the default unit types for everybody that uses the FontSizePicker component? The PR description suggests that the intent was the former, but the code changes here will do the latter. Let me know and I can advise on where to make the changes. |
|
🤔 Great question. I think these units are relevant enough to be added to the component... |
There was a problem hiding this comment.
I noticed that there is some isValueUnitRelative logic that determines whether to tweak the max and step values. vw and vh are both relative values — can we bunch them in with em and rem for this same logic, or do we want other values for max and step for these units?
gutenberg/packages/components/src/font-size-picker/index.tsx
Lines 253 to 254 in 6527034
I wouldn't consider this a "breaking" change so I don't mind either way, but I'm just wondering whether I also wanted to make sure it was clear that we're changing the defaults. Any consumer of FontSizePicker could've already passed |
|
For the range slider, it makes sense to me to limit the max value to 10 instead of 100, not least because starting at 50 would be very large. What about the native file? I probably should not have touched it @WordPress/native-mobile. |
|
Here is the range slider in the custom font size control in the Typography panel, with the decimals and max value at 10. Screen.Recording.2024-04-24.at.10.56.33.mp4 |
I'll take a look 👍 |
|
All working well on mobile, thanks for the ping! 🚀 |
|
Since I have not heard anything, I added vw and vh to |
|
Thanks for the updates. Range slider looks good! This is testing as expected for me, and I think a nice addition to the text block font size options. Also I can't find any regressions in the way 
I can't see any interference with fluid typography (which guards against units it doesn't understand anyway) 2024-05-09.10.54.17.mp4LGTM, but I'd just cc @WordPress/gutenberg-design for awareness and in case there are any gotchas. |
|
Seems good to me! |
|
Which is the correct place to add the change log entry? I understand that I need to move it to unreleased, but should it be under enhancements or internal? |
|
My vote would be under "Enhancements" 👍🏻 |
There was a problem hiding this comment.
I think we will also need to update the JSDoc and README for this component.
gutenberg/packages/components/src/font-size-picker/types.ts
Lines 29 to 34 in 31d9f7f
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
The only concern with this PR is, as already mentioned, changing the default units of the FontSizePicker itself. And as far as I know, unlike What do you think about changing the default units of the typography panel in anticipation of adding support for |
packages/components/CHANGELOG.md
Outdated
| @@ -45,6 +45,7 @@ | |||
|
|
|||
| ### Enhancements | |||
|
|
|||
| - `FontSizePicker`: Add `vw` and `vh` units to the font size picker ([#60507]((https://github.com/WordPress/gutenberg/pull/60607)). | |||
There was a problem hiding this comment.
Sorry, if you were to ship this PR as is, you would need to move the entry to the "Enhancements" section within the "Unreleased" section.
|
Without a way to control the units, it is not going to make a difference for either type of extenders where the units are added. |
|
I'm concerned about backward compatibility and extensibility. For example, the following changes do not require any changes to the diff --git a/packages/block-editor/src/components/global-styles/typography-panel.js b/packages/block-editor/src/components/global-styles/typography-panel.js
index 64a7be0443..75e5912d2b 100644
--- a/packages/block-editor/src/components/global-styles/typography-panel.js
+++ b/packages/block-editor/src/components/global-styles/typography-panel.js
@@ -196,6 +196,7 @@ export default function TypographyPanel( {
const resetFontFamily = () => setFontFamily( undefined );
// Font Size
+ const units = [ 'px', 'em', 'rem', 'vw', 'vh' ];
const hasFontSizeEnabled = useHasFontSizeControl( settings );
const disableCustomFontSizes = ! settings?.typography?.customFontSize;
const mergedFontSizes = getMergedFontSizes( settings );
@@ -385,6 +386,7 @@ export default function TypographyPanel( {
disableCustomFontSizes={ disableCustomFontSizes }
withReset={ false }
withSlider
+ units={ units }
size="__unstable-large"
/>
</ToolsPanelItem>This means that developers using And assuming that font size units are supported in theme.json in the future, all we need to do is make the following changes: diff --git a/packages/block-editor/src/components/global-styles/typography-panel.js b/packages/block-editor/src/components/global-styles/typography-panel.js
index 75e5912d2b..38c501d7ee 100644
--- a/packages/block-editor/src/components/global-styles/typography-panel.js
+++ b/packages/block-editor/src/components/global-styles/typography-panel.js
@@ -196,7 +196,8 @@ export default function TypographyPanel( {
const resetFontFamily = () => setFontFamily( undefined );
// Font Size
- const units = [ 'px', 'em', 'rem', 'vw', 'vh' ];
+ const [ availableUnits ] = useSettings( 'spacing.units' );
+ const units = availableUnits || [ 'px', 'em', 'rem', 'vw', 'vh' ];
const hasFontSizeEnabled = useHasFontSizeControl( settings );
const disableCustomFontSizes = ! settings?.typography?.customFontSize;
const mergedFontSizes = getMergedFontSizes( settings );What do you think about this approach? |
|
My opinion remains unchanged, that these units are relevant enough to be enabled as the default. |
|
@t-hamano is there any reason why |
My only concern is that for developers using the |
There was a problem hiding this comment.
If we update the CHANGELOG pointed out in this comment, we should be able to merge.
Add vw and vh to the list of default units in the font size picker. Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: geriux <geriux@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org>
Add vw and vh to the list of default units in the font size picker. Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: geriux <geriux@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org>
What?
Add the
vwandvhunits to the custom font size picker.Partial for #23323
Why?
The custom font size picker is limited to using px, em, rem. Users are asking for the units to be extended (The units are also more limited than in the custom spacing control, where px, em, rem %, vw and vh are supported)
How?
The PR updates the units in the font size picker component.
Testing Instructions
Add a text based block like a paragraph.
Open the block settings and open the Typography panel.
Select "Set custom size".
Confirm that vw and vh units can be selected.
Select them and confirm that the units are used in the editor and front.
Testing Instructions for Keyboard
Screenshots or screencast