-
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
Search block: reset size correctly when clearing unit control #65468
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +3 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
Flaky tests detected in 4d5c35e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10929307417
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice bug fix! Works as expected 👍
widthUnit === '%' && | ||
parseInt( newWidth, 10 ) > 100 | ||
? 100 | ||
: newWidth; | ||
const parsedNewWidth = parseInt( newWidth, 10 ); | ||
setAttributes( { | ||
width: parseInt( filteredWidth, 10 ), | ||
width: Number.isNaN( parsedNewWidth ) | ||
? undefined | ||
: parsedNewWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit nitty, but this makes it read like parsedNewWidth
can be an invalid value (NaN
), when in fact it was a valid value (empty string, aka undefined
).
I think making that explicit in the parsing stage will avoid raising alarm bells for the reader. For example:
const parsedNewWidth =
newWidth === ''
? undefined
: parseInt( newWidth, 10 );
setAttributes( { width: parsedNewWidth } );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll apply the suggestion and merge
4d5c35e
to
29d0d29
Compare
* Search block: reset size correctly when clearing unit control * Make empty string handling more explicit --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 80df28e |
What?
Related to #65340
Fix resetting the
width
of the Search block when clearing theUnitControl
value in the block inspector sidebarWhy?
The behaviour on
trunk
is buggy and doesn't reset correctly the widthHow?
There are two bug fixes:
onChange
callback for theUnitControl
component, the value is parsed from string to number, but is never checked forNaN
(which happens when the input is cleared). The new code sets the width back toundefined
instead ofNaN
ResizableBox
'ssize
prop was incorrect:width
andheight
properties, but onlywidth
was specified. Now,height
is specified too;width
was sometimes set toNaN
, but that's implicitly fixed with the changes to theUnitControl
'sonChange
handlerTesting Instructions
ButtonGroup
in the block inspector sidebarUnitControl
in the block inspector sidebarUnitControl
in the block inspector sidebar:Screenshots or screencast
Kapture.2024-09-18.at.22.04.10.mp4
Kapture.2024-09-18.at.22.02.08.mp4