BorderRadiusControl: Stop using UnitControl’s deprecated unit prop
#39549
Conversation
stokesman
added
[Type] Code Quality
|
Size Change: -4 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I was literally about to start working on this task, so extra thanks for opening this PR, @stokesman !
The code changes here LGTM and they test well as per instructions 🚀
I think there is a bug in this control. When the sides are unlinked and units vary but the radius values are all the same number, switching back to linked sides does not show the "Mixed" placeholder in the input.
I can confirm this is happening also for me.
I also came across another unexpected behavior while playing around this this component (in my case, in the global styles sidebar):
- I edited the border width and radius for the columns block, saved the global styles and reloaded the page
- If I delete the values for border radius (and width) in the
UnitControlinputs, the borders don't update as if I had removed them — to achieve that, I have to explicitly input0. The same behavior happens when "resetting" this fields through theToolsPaneldropdown. - Basically, I expected that deleting the value in these input would be equivalent to "delete" those values, instead of resetting them to the last saved status
borderradiuscontrol.mp4
Just pinging @aaronrobertshaw and @andrewserong who might be able to confirm if the two behaviors listed above are unintended — in any ways, they should be treated separately from this PR.
I'm glad you saw this PR first then! The root reason I made this is because I noticed #39531 didn't fix that issue for |
Thanks for the ping!
I think this also happens with
Yes, totally something for us to investigate separately 🙂 |
I think this is because getAllValue appears to do its best to display the value and uses But again, probably a good thing to explore in a separate issue / PR if we think it'd be good to change. This PR is testing well for me, though! |
From memory, this followed the
If the value was shown and the |
There was a problem hiding this comment.
While testing this I think I encountered a possible regression with the UX. I believe previously a user could select a non px unit prior to entering a value in the input field. At the moment, switching the unit immediately puts the input field in a Mixed state.
I don't think it is related directly to this PR and might have been introduced via recent changes around the UnitControl (e.g. #38987?).
@andrewserong is already on top of it and has a fix up in #39563.
I'd say this should be right to merge.
|
Thank you so much for the reviews!
That's some super fast turnaround! Thank you again! Regarding potential follow-ups: When the sides are unlinked and units vary but the radius values are all the same number, switching back to linked sides does not show the "Mixed" placeholder in the input.I confirm that is also how (also, I realised how similar the logic of this Clearing the quantity from the input causes the last saved/persisted value to be used, instead of acting as "I don't want any padding/border/etc". As a consequence, this happens also when resetting values from
|
I'd like to think there is some common functionality we could extract. A possible first step might just be to refactor the util functions used given the controls have different icons, slider and layouts for the unlinked controls etc. |
|
Agreed, @aaronrobertshaw ! Regarding this issue:
Would you or @andrewserong be able to follow up on this one? It's an area of the codebase that I don't know very well |
I'll follow up on this. Hopefully tomorrow. |
What?
Related to #39503 , this PR removes the use of
UnitControl’s deprecatedunitprop inBorderRadiusControl.Why?
The
unitprop is marked as deprecated, the component's docs recommend passing the unit directly through thevaluepropHow?
Remove the
unitprop on sub-componentAllInputControl. It is redundant since the values already contain the unit.Testing Instructions
*I think there is a bug in this control. When the sides are unlinked and units vary but the radius values are all the same number, switching back to linked sides does not show the "Mixed" placeholder in the input.