-
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
LineHeightControl: Custom spin buttons adjust from zero instead of base default #48766
Comments
@ciampo and @mirka it looks like we have a small regression here after the implementation of the I'm not sure of the best approach to solving this. Would it be possible to make the custom spin buttons in Alternatively, as a hacky quick fix option that works, we could wrap the LineHeightControl's Any suggestions? Quick fix diffdiff --git a/packages/block-editor/src/components/line-height-control/index.js b/packages/block-editor/src/components/line-height-control/index.js
index 670d6fac37..61e0c4a3f3 100644
--- a/packages/block-editor/src/components/line-height-control/index.js
+++ b/packages/block-editor/src/components/line-height-control/index.js
@@ -84,6 +84,15 @@ const LineHeightControl = ( {
? undefined
: { marginBottom: 24 };
+ const handleOnChange = ( nextValue, { event } ) => {
+ if ( event.type === 'click' ) {
+ onChange( adjustNextValue( nextValue, false ) );
+ return;
+ }
+
+ onChange( nextValue );
+ };
+
return (
<div
className="block-editor-line-height-control"
@@ -93,7 +102,7 @@ const LineHeightControl = ( {
{ ...otherProps }
__unstableInputWidth={ __unstableInputWidth }
__unstableStateReducer={ stateReducer }
- onChange={ onChange }
+ onChange={ handleOnChange }
label={ __( 'Line height' ) }
placeholder={ BASE_DEFAULT_VALUE }
step={ STEP }
|
There are also some other quirks/bugs with the LineHeightControl currently that I've detailed in a separate issue (#48768) as they are less significant. That said, it might be worth considering them when assessing potential solutions. |
As we discussed in the original PR thread, I do think that the cleanest solution within the current implementation is to move the spin buttons down into the InputControl component and hook it up to the reducer system. This LineHeightControl regression (and potentially in other similar consumers) is another good reason to do that. @stokesman has floated the idea of an overhaul to the reducer system, but barring that I think it's probably worth moving the spin buttons down. (Not that I'm opposed to your quick fix if it's urgent.) |
Can't tell you how many times I've been snagged here. I'd like to get a quick fix in, then circle back to clean up. |
I'm not clear on how we would best achieve this. I'm guessing we'd need to lift the state reducers from The amount of refactoring there, makes it seems to me like we should just refactor the component to the hook/component pattern as @stokesman proposed.
Given the frequency with which this bug catches people out, I have put up a PR to apply the quick fix at the |
Related:
Description
The custom spin buttons for the
LineHeightControl
do not increment or decrement from the default1.5
represented in the control's placeholder value. Instead they increment/decrement from0
.This is a regression from the behaviour prior to when the custom spin buttons were added to the
NumberControl
in #45333.Expected Behaviour
The spin buttons work as they did prior to #45333 i.e. when the field is initially unset and displaying the
1.5
placeholder value, clicking either custom spin button should adjust that placeholder value not zero.Step-by-step reproduction instructions
0.1
as its value instead of1.6
.0
.Screenshots, screen recording, code snippet
Screen.Recording.2023-03-06.at.5.13.20.pm.mp4
Screen.Recording.2023-03-06.at.5.15.04.pm.mp4
Environment info
Please confirm that you have searched existing issues in the repo.
Yes
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Yes
The text was updated successfully, but these errors were encountered: