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

UnitControl: show unit label when units prop has only one unit #40784

Merged
merged 6 commits into from
May 4, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented May 3, 2022

What?

Fix an issue in UnitControl, where the unit label was not being shown if the units array prop only had one item.

Why?

This is a bug and should be fixed. See #40697 (comment) and #40697 (comment) for more context.

How?

The fix is to use the label of the single unit passed through the units prop as the initial unit value (as suggested in this comment by @stokesman ).

This PR also restores the should render label if single units unit test to how it used to be before the changes applied in #40697.

Testing Instructions

  • Make sure tests pass

  • Visit the "Single Unit Controlled" Storybook example on trunk, verify that there's no unit being displayed when the Story initially renders. Check that the unit is only shown after a numeric value is entered in the input field.

  • Apply this patch below to restore the same Storybook example on this branch, visit the same "Single Unit Controlled" Storybook example, and verify that the initial unit is being displayed during the Story initial render.

Click to show the patch
diff --git a/packages/components/src/unit-control/stories/index.tsx b/packages/components/src/unit-control/stories/index.tsx
index 87dfd3a84c..de60479e7b 100644
--- a/packages/components/src/unit-control/stories/index.tsx
+++ b/packages/components/src/unit-control/stories/index.tsx
@@ -117,6 +117,29 @@ WithSingleUnit.args = {
 	units: CSS_UNITS.slice( 0, 1 ),
 };
 
+export const WithSingleUnitControlled: ComponentStory<
+	typeof UnitControl
+> = ( { onChange, ...args } ) => {
+	const [ value, setValue ] = useState< string | undefined >( undefined );
+
+	return (
+		<div style={ { maxWidth: '100px' } }>
+			<UnitControl
+				{ ...args }
+				value={ value }
+				onChange={ ( v, extra ) => {
+					setValue( v );
+					onChange?.( v, extra );
+				} }
+			/>
+		</div>
+	);
+};
+WithSingleUnitControlled.args = {
+	...Default.args,
+	units: CSS_UNITS.slice( 0, 1 ),
+};
+
 /**
  * It is possible to pass a custom list of units. Every time the unit changes,
  * if the `isResetValueOnUnitChange` is set to `true`, the input's quantity is

Screenshots or screencast

Before

unit-control-single-label-before.mp4

After

unit-control-single-label-after.mp4

@ciampo ciampo force-pushed the fix/unit-control-single-unit-label branch from 5b4697a to 08b3d29 Compare May 3, 2022 17:23
@ciampo ciampo marked this pull request as ready for review May 3, 2022 17:28
@ciampo ciampo requested a review from ajitbohra as a code owner May 3, 2022 17:28
@ciampo ciampo self-assigned this May 3, 2022
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components [Feature] Component System WordPress component system labels May 3, 2022
@@ -109,40 +109,10 @@ TweakingTheNumberInput.args = {
/**
* When only one unit is available, the unit selection dropdown becomes static text.
*/
export const WithSingleUnitControlled: ComponentStory<
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just undoing some Storybook changes that were left from previous work on #40697

render(
<UnitControl
units={ [ { value: '%', label: '%' } ] }
value="30%"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing value prop from test, which was added as a temporary fix in #40697 (see #40697 (comment) for context)

@ciampo ciampo force-pushed the fix/unit-control-single-unit-label branch from 77cbc41 to c86a634 Compare May 3, 2022 21:13
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up @ciampo 👍

✅ Tests pass
✅ All Storybook examples for UnitControl function as expected
✅ Works as expected in editor and theme setting only single unit

I did have one minor question/tweak that I've left as an inline comment. Happy to review again and give final approval once that's resolved.

packages/components/src/unit-control/index.tsx Outdated Show resolved Hide resolved
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

✅ Tests still pass
✅ UnitControl behaves as advertised within Storybook examples
✅ Still working in the editor with varying custom unit configurations

I'd say this should be right to land once all the checks are green. 👍

@ciampo ciampo merged commit 3ce2c79 into trunk May 4, 2022
@ciampo ciampo deleted the fix/unit-control-single-unit-label branch May 4, 2022 14:24
@github-actions github-actions bot added this to the Gutenberg 13.2 milestone May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants