LineHeightControl: Allow for more granular control of decimal places #52902
Conversation
mikachan
added
[Package] Block editor
|
Size Change: +36 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1984787. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5679123569
|
|
I don't think this is a good idea. Pressing 10x more times to get + 0.1 point change doesn't seem ideal. The current implementation is And if you want to go from 1.5 to 1.7, it's quite a heavy UX lift, whereas today it's only two clicks. |
I was about to comment the same. I think the best solution would be to make it possible to enter values |
mikachan
changed the title
|
Thanks, both, for the feedback! I agree; whilst that solution allowed for 2 decimal places, it introduced a UX regression. I've updated the PR with another idea, which allows the user to manually enter up to 2 decimal places but will also maintain the step value of The value in the So I've introduced an optional prop that can be passed to the |
|
Thanks @mikachan ! For context, for most font sizes it's common to have rounded values. However, sometimes it's needed to have the second decimal number. That's what this PR is for. For the input field controls, I'd go with the rounding. Examples: https://typescale.com/ |
This is a limitation because of how the block was originally introduced; it's manually applying CSS classes for every tenth value. 😅
Looks ace! |
There was a problem hiding this comment.
Thank you for taking this up 🙌 The current changes do resolve the issue. While it doesn't cause practical problems that I'm aware of, this part bugs me:
That's Chrome on macOS if you hover the input. This is due to step being more a validation attribute than anything else.
Because of that, I think your first pass of setting the line height control’s step to .01 was the way to go. Since the granularity of the stepping UX should be preserved, we could introduce a prop to specify that instead. I'd feel better about that direction. WDYT?
A diff of quick and dirty changes toward suggested direction
diff --git a/packages/block-editor/src/components/line-height-control/index.js b/packages/block-editor/src/components/line-height-control/index.js
index dc0cf05d18..62233c9d93 100644
--- a/packages/block-editor/src/components/line-height-control/index.js
+++ b/packages/block-editor/src/components/line-height-control/index.js
@@ -111,8 +111,8 @@ const LineHeightControl = ( {
onChange={ handleOnChange }
label={ __( 'Line height' ) }
placeholder={ BASE_DEFAULT_VALUE }
- step={ STEP }
- roundingStep={ ROUNDING_STEP }
+ step={ ROUNDING_STEP }
+ spincrement={ STEP }
value={ value }
min={ 0 }
spinControls="custom"
diff --git a/packages/components/src/number-control/index.tsx b/packages/components/src/number-control/index.tsx
index 1909937b97..bbb900ec4e 100644
--- a/packages/components/src/number-control/index.tsx
+++ b/packages/components/src/number-control/index.tsx
@@ -43,7 +43,7 @@ function UnforwardedNumberControl(
required = false,
shiftStep = 10,
step = 1,
- roundingStep,
+ spincrement = step,
type: typeProp = 'number',
value: valueProp,
size = 'default',
@@ -65,7 +65,8 @@ function UnforwardedNumberControl(
const mergedRef = useMergeRefs( [ inputRef, forwardedRef ] );
const isStepAny = step === 'any';
- const baseStep = isStepAny ? 1 : ensureNumber( step );
+ let baseStep = ensureNumber( spincrement );
+ if ( spincrement === step ) baseStep = isStepAny ? 1 : ensureNumber( step );
const baseValue = roundClamp( 0, min, max, baseStep );
const constrainValue = (
value: number | string,
@@ -73,15 +74,11 @@ function UnforwardedNumberControl(
): string => {
// When step is "any" clamp the value, otherwise round and clamp it.
// Use '' + to convert to string for use in input value attribute.
- return isStepAny
- ? '' + Math.min( max, Math.max( min, ensureNumber( value ) ) )
- : '' +
- roundClamp(
- value,
- min,
- max,
- stepOverride ?? ( roundingStep || baseStep )
- );
+ return `${
+ isStepAny
+ ? Math.min( max, Math.max( min, ensureNumber( value ) ) )
+ : roundClamp( value, min, max, stepOverride ?? baseStep )
+ }`;
};
const autoComplete = typeProp === 'number' ? 'off' : undefined;
I mentioned earlier why .01 stepping holistically would make line height + and - quite tedious. I wouldn’t want to press + numerous times to really modify the line height. |
|
My bad if I didn't make that very clear but in part of what you quoted:
Meaning that the direction I've suggested maintains the current UX. |
Co-Authored-By: Mitchell Austin <9000376+stokesman@users.noreply.github.com>
|
Thanks all, for all the feedback and input! 🙇 I really like the I think this is ready for another review! |
|
Thanks for the updates Sarah!
I agree with fixing the issue Chad pointed out and ideally doing so in A diff fixing the spin issue in
|
|
Hannes, thanks for bringing that up again. I did see and consider your earlier comment yet it seems quite arbitrary. I.e. it's not clear which behavior is favorable in common use.
As example of why this seems arbitrary, this alternative way is as quick or quicker: I’m not really opposed to it but I think we might hold off on specializing it that way for the moment. |
|
@stokesman I think the biggest challenge on deciding how the component should behave is that the plus and minus are visually very prominent and the element is not not known for it's behaviour yet.
I agree and can see that people expect them to change the value by 0.1 (meaning
This is a very valid point emphasises the argument. I guess I have change my mind and agree with you that we should (for now) avoid a special rounding. |
Co-Authored-By: Mitchell Austin <9000376+stokesman@users.noreply.github.com>
Co-Authored-By: Mitchell Austin <9000376+stokesman@users.noreply.github.com>
|
Thanks both! I really appreciate the speedy feedback.
I'm not too opinionated either way, but I also agree here, and it's potentially complicating things by specialising the logic, at least for now. I've tried out both of @stokesman's diffs from the above comment, and I really like how this is working. I agree that the new prop ( |
There was a problem hiding this comment.
Thanks again for the updates! I've taken it for another 🙊spin and everything behaves as expected. I noticed there were some unit tests that failed and manually tested to verify the behaviors still work. It was merely an implementation detail in the tests that needed an update. I pushed a commit for that so the tests should pass now.
Maybe we want sign off from one of the components crew but this looks good to go. I ❤️ the feature being added to NumberControl so the finer UX can be supported. Thanks again for tackling this Sarah 🙌 .
|
Thanks for updating that unit test! I stepped away from my computer before the tests finished yesterday and didn't run them locally 🙈 Thanks also for all your help with this PR! |
|
@mirka Would you be able to review this please, as someone who knows the components well? Or suggest a good person for a review. Thank you 🙇 |
There was a problem hiding this comment.
Loved seeing the collaboration here 🚀
To be honest though I'm a little confused as to what the ultimate change is supposed to be here 🤔 This is the behavior I'm seeing, and it seems to contradict what's written in the code comments ("A line-height value of 1.55 will increment to 1.65"):
CleanShot.2023-08-11.at.06.15.38.mp4
What exactly is supposed to happen after the changes in this PR?
In that vein, I think it'd be good to add some tests for the new behavior as well.
I just double-checked and don't see what you are seeing. Perhaps there was a build hiccup of some sort. This is what I'm seeing: Google.Chrome.-.Add.New.Post.My.WordPress.Website.WordPress.2023-08-10.at.4.38.07.PM.mp4
Simply that the line height control accepts numbers with a hundredth decimal place and its behavior remains otherwise unchanged. It already "spins" by tenths and showing that it still does that is proof of preserved UX.
That's a fine idea. I'm not sure we'd need one for |
Co-authored-by: Lena Morita <lena@jaguchi.com>
Co-authored-by: Lena Morita <lena@jaguchi.com>
|
Thanks @mirka! To confirm, I'm seeing the same as @stokesman on this branch, and the biggest difference is that users can now manually add and save a line-height value with a hundredth decimal place. Currently, this decimal place is removed on blur or when interacting with the spin controls. Here's an example from trunk to compare to @stokesman's video: Screen.Recording.2023-08-11.at.14.30.46.movI've added some tests for the |
|
Woop, thanks @mirka! I'll bring this in 🎉 (I had loads of fun working on this, thanks everyone!) |
ciampo
added
the
has dev note
|
Added a dev note in the PR description |
What?
This adds an additional
stepvalue to theLineHeightControlof0.01, which allows for more granular control over the line-height value.Currently, users are only able to use values with single decimal places, such as 1.5 or 1.6. But there is quite a big difference between just one decimal place, and it would be great if smaller values could be set, such as 1.55, etc.
Please see #42998 and #51754 for more details.
Why?
Closes #42998.
How?
Adds a
spinFactorprop to the NumberControl component to opt-in for more granular control over decimal places.Testing Instructions
Screenshots or screencast
Screen.Recording.2023-07-27.at.10.35.32.mov
Dev note
A new
spinFactorprop has been added toNumberControl, allowing consumers of the components to specify by how much should the input's value increment/decrement with respect to thestepprop. This is particularly useful when more granular control is needed (thus allowing for more decimal places and a smallerstep) but without sacrificing the UX around manually incrementing/decrementing the value.Even if the prop was added to
NumberControl, all components based onNumberControlcan benefit from this change — this includes, for example,UnitControlandLineHeightControl.