Border Radius Control: Fix undefined value on first click into RangeControl #35651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
|
Size Change: +1 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for very quick debugging and fix @andrewserong 🏅
I could replicate the issue as described and after applying this PR branch and following the test instructions the issue has been fixed.
I'd like to suggest a small alternative option. We could just add the fallback to where unit is first calculated. This would also allow the max value to work properly as well in the case where the unit is missing.
const unit = getAllUnit( values ) || 'px';What do you think?
Oh, good catch, thanks @aaronrobertshaw! I've just pushed this change. |
aaronrobertshaw
approved these changes
Oct 15, 2021
andrewserong
deleted the
fix/border-control-handle-slider-change-add-fallback-unit
branch
|
Thanks @andrewserong and @aaronrobertshaw for the fix ❤️ I was wondering if we could add some (unit) test coverage for added stability (to avoid e.g. people wanting to change the default from Would it make sense to maybe move the |
That's a great idea @ockham, thanks for suggesting it. Now that this fix is in, I'm happy to look into that as a follow-up next week to tidy it up and improve stability. I'll add it to my list! |
|
Thanks @andrewserong, sounds great! 😄 |
7 tasks
|
I've added a follow-up PR that includes unit tests for the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #35628
As reported by @ockham, in blocks that opt-in to the border radius control, if you click to set a border radius using the
RangelControl(the slider) from a fresh state where there isn't already a value, then the value in the code editor view will appendundefinedto the end of the value, instead of a fallback default unit that we'd expect (px).This PR updates the border radius control's
handleSliderChangemethod to treat the current unit aspxif a unit is not currently set.How has this been tested?
Manually in the editor.
pxvalue (beforehand, you'd see a value like41undefinedwhereas now it should be41px)Smoke test that this change doesn't affect switching between units, or the behaviour of the UnitControl.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).