-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Block Library - Cover]: Fix keep selected unit on deleting minHeight value #39145
[Block Library - Cover]: Fix keep selected unit on deleting minHeight value #39145
Conversation
Size Change: +71 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
Thanks for looking at this!
I can confirm that this has been fixed in this PR, fixtures pass 🎉 and also there are no block validation errors for the following block code (pulled from trunk) No value for % unit (from trunk)```htmlBeforeAfter
I can confirm that Would it make sense to also add a default value for
That appears to fix the UX problem of no default value of |
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.
This tested well for me:
- Could remove minHeight value and unit persisted
- Previous blocks with
style="min-height:undefined%"
migrated successfully and loaded without invalidation errors - Previous cover blocks with a range of background, height, opacity settings all loaded without any validation errors
- Checked that the bug that
onUnitChange()
was introduced to fix was not reintroduced - and it isn't
Agree with @ramonjd that it might make sense to add a default value for %
.
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.
This LGTM, too, thanks for fixing this up @ntsekouras! That feels much nicer to work with now that it doesn't switch the unit to px
👍
Same testing as Glen and Ramon, removing minHeight
value now allows the unit to persist, previous block with the min-height:undefined%
value migrated successfully, and all the deprecated block markup migrates successfully.
One tiny thing to add, if we go the route of adding in default values (that sounds good to me, too), just a heads-up that with the change in #38987 it'll be expected (but not enforced) that default values should be numbers and not strings, I believe. So I think it'd be:
defaultValues: { px: 430, '%': 20, em: 20, rem: 20, vw: 20, vh: 50 },
#38987 just got merged! Would it be possible to rebase and apply Andrew's suggestion?
Thank you! |
Thanks for all the reviews. I'll do that soon! |
108b0d1
to
0cb7183
Compare
Still tested well for me, and selecting % defaulted to a value of 20 now. |
Part of: #38872
When you insert a Cover block and change the
min height
unit and then delete the value, theunit
changes back topx
.This PR is part of the issue and is about the
minHeight
control in Cover block. In the issue there is reference to Image block and border radius, that I haven't looked into.There was another bug that wasn't impactful and due to the
default
values per unit was only occurring when%
was selected. In that case we would have aunit
but novalue
and the markup would include invalid css likemin-height:undefind%
. This PR handles that as well and I added a deprecation.Testing instructions
Before
revert.mov
After
Screen.Recording.2022-03-01.at.7.23.17.PM.mov