-
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
BorderBoxControl: Omit unit select when values are mixed #44592
BorderBoxControl: Omit unit select when values are mixed #44592
Conversation
Makes complete interface sense to me, as showing the unit would be misleading. 👍 👍 — probably just needs a code sanity check. Thank you! |
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.
I believe this definitely goes in the right direction.
While testing in storybook, I noticed a few unexpected behaviors. In particular, if the 4 borders have same width but different color or style, when linking them the unit is still hidden, while the "quantity" is shown — to me that's a bit confusing
Kapture.2022-09-30.at.18.00.09.mp4
As a general rule, we should either:
- always show the "mixed" (i.e. unset) state when the 4 split borders are not exactly the same
- apply this logic separately for border width (quantity+unit), border color and border style
For example, in the scenario above, when switching from un-linked to linked, I would have expected to either see "1px" or "Mixed" (without unit).
Hope this makes sense! Happy to clarify otherwise
Good catch, thanks. It's a minor change to make the disabling of the unit select depend only on the border width being mixed. I think this gives us the best of both worlds. Here's a quick video of the component as of aeb352b Screen.Recording.2022-10-04.at.5.53.46.pm.mp4 |
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.
I agree, this looks better!
Up to you if you want to add a unit tests for this new behaviour, but otherwise LGTM 🚀
Thanks for the nudge; I'll add some tests tomorrow 👍 |
Size Change: +2.87 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
Related:
What?
Hides the unit select from the
BorderBoxControl
when it has mixed values.Behind the scenes the control still maintains the most common unit selection from the mixed values. This way when the user adjusts the "linked" control then that previous unit selection is honoured.
Why?
How?
Adds a new
disableUnits
prop to theBorderControl
, which is passed through to theUnitControl
. When theBorderBoxControl
has mixed values it will use the new prop to disable the unit select.Alternative approach.
One alternative might be to simply check for the existence of a placeholder in the
BorderControl
. At preset the only time this placeholder is set is when theBorderBoxControl
passes 'Mixed' to represent that state. This seemed a bit fragile and I can see in the near future if we gain access to Global Styles data in the post editor, that value might actually be used as a placeholder.Testing Instructions
Screenshots or screencast
Screen.Recording.2022-09-30.at.12.05.42.pm.mp4