BorderBoxControl: Ensure most common unit selection is maintained #44565
Conversation
This file contains hidden or 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
aaronrobertshaw
added
[Type] Bug
|
Size Change: +266 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Sep 29, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Changes LGTM and test well as per instructions — also extra kudos for the additional unit tests!
My only doubt in terms of UX is the discrepancy in the logic when the controls are re-linked:
- we make an opinionated choice for the unit
- we keep the width and the style "mixed"
Also, Since BorderBoxControl's logic looks very similar to the one in BoxControl, BorderRadiusControl (in the @wordpress/components package) and SpacingSizesControl (in the @wordpress/block-editor package), are these changes something that should be applied to those components as well?
Not for this PR, but @mirka and I had already discussed with @glendaviesnz about working on a base component that can abstract the common parts that these components share. It would be great if we could start planning for that too
Contributor
Author
|
Thanks for the review @ciampo 👍
I had similar doubts as well but if we are to keep the unit select around when the control has mixed values, this PR's approach felt the most natural to me. If the user has gone to the trouble of setting non-px units, it's a little annoying to switch back to the linked view and go to "unify" the border widths but have to reselect the unit again because it was set to All that said, I'm happy to adjust it in a follow-up if anyone has a better option. It is also worth noting that the potential removal of the unit select when the control has mixed values was floated in #41860. In that PR, I'm trying to increase the border controls to 40px in height. The increased padding that comes with that size variant means we no longer have space for the 'Mixed' placeholder, for example. Removing the unit select in that situation would free up some real estate. For this component, it would also then make the unit "mixed" as per width value, color, and style.
I believe those components already maintain the most common unit as the selected unit when they have mixed values.
Agreed. I had those same discussions with Glen when he started out on the spacing presets UI. I'll happily take a run at it once I have the bandwidth if no one else has already picked it up. |
aaronrobertshaw
deleted the
fix/border-box-control-most-common-unit-selection
branch
Contributor
|
For sure this bit of UX is tricky, happy to iterate in the future as we keep using this component and gaining more insights |
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.
Related:
What?
Updates the
BorderBoxControlsuch that when it contains "mixed" border values, it maintains the most common unit selection when switching back to the "linked" view.Why?
Having the "linked" control display 'px' as the selected unit when split borders might all be using different CSS units is confusing. This PR brings it more in line with the box control while still maintaining the border control specific behaviour.
How?
getCommonBorderutil to return a "CSS unit-only" value for mixed widths so theUnitControlmaintains an appropriate unit selection.Testing Instructions
BorderBoxcontrolunit tests:npm run test:unit packages/components/src/border-box-control/testScreenshots or screencast
Screen.Recording.2022-09-29.at.3.37.15.pm.mp4
Screen.Recording.2022-09-29.at.3.45.46.pm.mp4