Skip to content
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

Closed
aaronrobertshaw opened this issue Mar 6, 2023 · 5 comments · Fixed by #49150
Closed
Assignees
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@aaronrobertshaw
Copy link
Contributor

aaronrobertshaw commented Mar 6, 2023

Related:

Description

The custom spin buttons for the LineHeightControl do not increment or decrement from the default 1.5 represented in the control's placeholder value. Instead they increment/decrement from 0.

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

  1. Edit a post, select a paragraph, and switch to the styles tab in the Block Inspector
  2. Toggle on the line-height option from the Typography panel's menu
  3. Click the + button and note that the field now has 0.1 as its value instead of 1.6.
  4. Reset the line height control via the Typography panel's menu
  5. Toggle it back on and this time click the - button. Note the value in the field is now 0.

Screenshots, screen recording, code snippet

Actual Expected
Screen.Recording.2023-03-06.at.5.13.20.pm.mp4
Screen.Recording.2023-03-06.at.5.15.04.pm.mp4

Environment info

  • WP 6.1
  • Gutenberg trunk

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

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Mar 6, 2023
@aaronrobertshaw
Copy link
Contributor Author

@ciampo and @mirka it looks like we have a small regression here after the implementation of the NumberControl custom spin buttons in #45333.

I'm not sure of the best approach to solving this. Would it be possible to make the custom spin buttons in NumberControl trigger one of the InputControl actions so that everything continues to pass through the state reducers and we pick up the old behaviour again?

Alternatively, as a hacky quick fix option that works, we could wrap the LineHeightControl's onChange so it can pass the value through the same adjustNextValue function used by the state reducer. I don't feel this is a great option as I could see other consumers of NumberControl using the custom spin buttons and wanting to add their own custom state reducer.

Any suggestions?

Quick fix diff
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 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 }

@aaronrobertshaw
Copy link
Contributor Author

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.

@mirka mirka self-assigned this Mar 7, 2023
@mirka
Copy link
Member

mirka commented Mar 7, 2023

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.)

@mirka mirka removed their assignment Mar 7, 2023
@richtabor
Copy link
Member

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.

@aaronrobertshaw
Copy link
Contributor Author

move the spin buttons down into the InputControl component and hook it up to the reducer system

I'm not clear on how we would best achieve this. I'm guessing we'd need to lift the state reducers from InputField to the root InputControl so they can then be shared between InputField and InputBase, which would need to render the custom spin buttons.

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.

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.

Given the frequency with which this bug catches people out, I have put up a PR to apply the quick fix at the LineHeightControl level. That should buy us some time to address the root issue in a more thorough manner.

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants