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

Fixed Input Unit Block: Unit jumps to px when set to 0 #68212

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

Mayank-Tripathi32
Copy link
Contributor

@Mayank-Tripathi32 Mayank-Tripathi32 commented Dec 20, 2024

fixes #51616

What?

This PR fixes the issue where the unit in the spacer and dimension controls resets to px when the input value is cleared.

Why?

When removing the value from input fields like spacer height or padding in the dimensions control, the unit automatically resets to px. This behavior causes unexpected results and reduces user control over custom styles. Fixing this improves usability and ensures a consistent editing experience.

How?

The fix was applied in packages/components/src/unit-control/index.tsx, where the unit was being dropped when the input value was cleared. The implementation now ensures that when the value is empty, it defaults to 0 with the current unit preserved, preventing the reset to px.

Testing Instructions

  1. Open the WordPress editor (post or page).
  2. Add a Spacer block inside a stack.
  3. Change its unit to vh.
  4. Clear the height value to input a new value.
  5. Ensure the unit stays as vh and does not revert to px.

Additional Test:

  1. Go to Global Styles settings.
  2. Select the Button block.
  3. Adjust padding values and clear the input fields.
  4. Ensure the unit remains consistent and doesn’t default to px.

Screencast

test-unit-controls.mp4

@Mayank-Tripathi32 Mayank-Tripathi32 marked this pull request as ready for review December 20, 2024 20:18
Copy link

github-actions bot commented Dec 20, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mayank-Tripathi32 <[email protected]>
Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: stokesman <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: hanneslsm <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mayank-Tripathi32
Copy link
Contributor Author

If the proposed changes seem correct, I can update the failing unit tests in each component to handle value instead of undefined. This adjustment will align the tests with the updated behavior and ensure they pass as expected.

@Mayank-Tripathi32 Mayank-Tripathi32 changed the title Fixed Spacer Block: Unit jumps to px when set to 0 Fixed Input Unit Block: Unit jumps to px when set to 0 Dec 20, 2024
@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. labels Jan 13, 2025
@Mayank-Tripathi32
Copy link
Contributor Author

If the proposed changes seem correct, I can update the failing unit tests in each component to handle value instead of undefined. This adjustment will align the tests with the updated behavior and ensure they pass as expected.

It seems the test cases expects the default behaviour of "undefined", So I think its better to introduce a prop "shouldPreserveUnit" and conditionally add it for required places.

Copy link
Contributor

@yogeshbhutkar yogeshbhutkar left a comment

Choose a reason for hiding this comment

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

Works as expected ✨

Screen.Recording.2025-01-20.at.4.54.56.PM.mov

@stokesman
Copy link
Contributor

Works as expected ✨

The screen recording you posted doesn’t show properly testing #51616, which requires the Spacer block to be inside a Row/Stack. The issue wasn’t very clear about that. I missed it too #51616 (comment) when I was trying to reproduce the issue initially.

I'm not doubting this PR fixes the issue. The description includes a screen recording demonstrating an actual test of the issue.


@Mayank-Tripathi32, Why does fixing this require changes to UnitControl? I ask because the issue is not present in all instances of where the component is used. For one, the Spacer block’s innate height control (when not inside the row/stack) does not have the issue. For other examples try a BoxControl story or BorderBoxControl story. They both have a UnitControl but do not have the issue. To me this indicates that it’s probably not a bug in UnitControl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spacer block: broken UX in clearing its "fixed" width/height input when inside a row/stack
4 participants