Border Controls: Update control components to allow 40px height #41860
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: -8 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
update/border-controls-to-40px-height
branch
2 times, most recently
from
e74524c
to
e843ae8
Compare
|
This PR has been updated to also include turning on the 40px high input in our editors as per this request. We've recently updated the styling for large control variants within the editors to increase internal padding. For example, the This is a bit of a problem currently for the border controls as they use both a prefix and a suffix. When also constrained by the width of our sidebar there isn't much space to actually display entered values. This is accentuated further in the split view, configuring individual sides. Click for demo of above issue before tweaksScreen.Recording.2022-09-26.at.2.51.36.pm.mp4I've taken an initial pass at trying to tweak the control styles and input widths to maximise the space available without overriding the UnitControl's padding so as to keep it in line with other controls. It's better but there's still a lot more that needs improving. @jasmussen or @jameskoster, if you have time to cast an eye over this and provide your thoughts and suggestions, it would be greatly appreciated 🙇 Click for demo of current state in this PRScreen.Recording.2022-09-26.at.2.55.33.pm.mp4 |
|
Thanks for the ping @aaronrobertshaw. I just mocked this up in Figma (aligned to the grid we've been working towards) and everything seems to fit okay:
At a quick glance, I think we can increase the width of the inputs in the unlinked view, but ideally we should aim to fit the grid. In case it's helpful, here's the Figma file to inspect which should include all relevant values. |
|
Thank you for working on this, this is excellent attention to detail 🙏 Jay summarizes well. I'll only add — and this may not be needed for this PR — that it'd be nice of the "Swatch box" could be square. This one:
|
I thought it was, good spot 😅 |
Thanks for the updated mockup and figma file @jameskoster 👍 I'll massage the component into shape today.
This was something I'd tweaked locally but exacerbated the issue of overly constraining the display of the input value so I left it out for this first pass. It's an easy change if we are able to increase the overall input width. |
|
I've given the border control styles another pass. It's getting closer, but there are a couple of minor issues still for it to be pixel perfect. The figma file suggests the panel is Regardless of whether the inputs were
The
I looked at removing the unit select when the border control has a mixed set of borders however this felt rather odd when it was just a color difference causing the mixed state. The control was designed so even if the colors were different you could still adjust the width value and unit as desired. The border radius control has also been updated so it appears more in line with the main border control. Here's a quick video of the current state of these controls. Screen.Recording.2022-09-27.at.3.44.24.pm.mp4Any thoughts on ways to improve the controls from here? |
|
For the truncation issue, I think the best solution would be to remove the padding on the unit button. Doing so ensures 24x24px dimensions which are acceptable, and creates just enough space for
Ideally that wouldn't be an override for just this control. Do we need a new button variant? It doesn't look like we have a simple no-padding, no-border option 🤔 This doesn't quite create enough room for the 'Mixed' value though. Must it say 'Mixed', or is 'Mix' adequate? That fits:
The only other option I can think of would be to hide the unit input when the values are mixed... it doesn't seem to work properly at the moment anyway 🙈 |
packages/block-editor/src/components/border-radius-control/style.scss
Outdated
Show resolved
Hide resolved
aaronrobertshaw
force-pushed
the
update/border-controls-to-40px-height
branch
from
d90c6b3
to
a10eb32
Compare
|
Appreciate the continued feedback @jameskoster 🙇
I haven't pushed the removal of padding from the unit select button yet as I got caught up exploring the potential removal of the unit select button when we have mixed values. That said, it does provide just enough breathing room for those longer values. The plan is to also strip the unit select's padding from the radius control as well so at least the two controls that are next to each other in that panel match up.
I'm not sure if 'Mix' is adequate, it seems like a poorer option to me than 'Mixed', however, I think we are short of alternatives at this point. At least in terms of placeholders. On top of that, languages other than English will likely struggle to find something meaningful that is 3 characters or less.
The That would be better as a separate PR focused on those changes independent of the visual tweaks here. I'll publish a PR for it tomorrow. |
Could another option be a wildcard label, IE "*"? I'm not sure how intuitive that would be though. Perhaps with a tooltip it's be ok?
Another thought... does the linked control really need to support mixed values? What if toggling away from unlinked borders re-linked them automatically? |
That's an interesting idea. It might not be 100% intuitive but I don't know that it is any worse than "Mix" or whatever non-English placeholders get truncated to. The addition of the tooltip might be a little bit trickier. I think we'd need to be able to apply the tooltip to the UnitControl's inner input as we already have a tooltip on the color/style dropdown. Without being able to target the inner input we'd end up with multiple tooltips when the color and style dropdown was hovered over. There's a draft PR up that explores adding a tooltip for an
I think it might cause some friction if this control behaves differently than those around it. The spacing controls for padding, margin etc, box controls, and the border radius control, all allow that mixed state without "re-linking" the values. I'm also aware of users relying on that link button to simply collapse the control in the sidebar so it doesn't take up so much space. Now the inspector controls sidebar primarily uses the There's been discussion around adding a way to collapse ToolsPanels but they tend to fizzle out due to crowding the panel with buttons or obscure behaviour. Instead of a tooltip, could we leverage some help text? This might provide a way to;
From an a11y point of view, might it also provide an improvement over the simple placeholder?
|
|
Thanks for all the work here, this is a strong improvement! About the "Mixed" issue, I would avoid showing values in helper text. The asterisk could work. But I would think the strongest solution is to hide the "px" unit and show the regular "Mixed" text in the input (the units can be mixed too, afterall). Even if we have to elide the text to "Mix.", or simply allow it to be cropped by the input field, that still feels the most clear and simple. That's not to say we can't come back to this with stronger solutions, at a later point. It may also be possible to earn some space back by exploring a design that omits the separator between border indicator and value:
But that's for a different time! |
|
This is a good one for @mirka to check once she's back from AFK |
|
Thank you to everyone for all the feedback, thoughts, and ideas 🙇 I've switched the border controls back to using the 'Mixed' placeholder (962a853). I'll create a separate PR removing the unit select when there are mixed values. |
aaronrobertshaw
requested review from
ajitbohra and
ellatrix
as code owners
This allows us to remove most of the changes that would have been introduced in this PR as well as simplify the CSS.
aaronrobertshaw
force-pushed
the
update/border-controls-to-40px-height
branch
from
072f6af
to
832c018
Compare
|
@aaronrobertshaw happy to merge this PR today in case it gets late for you |
Thanks for handling the merge. The tests were still running when I had to log off for the day. Great to see it merged when logging in 🎉 |
Related:
What?
Adds the ability for the
BorderControl,BorderBoxControl, andBorderRadiusControlcomponents to be displayed with a40pxheight.At this stage, this PR does not toggle the display of border controls to
40pxby default. If we wish to make these controls that height straight away we can close this PR in favor of more straightforward tweaks.Why?
The current design direction for new tools / controls is to have a
40pxdefault height.How?
__next36pxDefaultSizeprop for theBorderControlandBorderBoxControlcomponents with__next40pxDefaultSizeBorderRadiusControlto accept a new__next40pxDefaultSizeprop__next40pxDefaultSizeprops to make each component display with a40pxheightBorderRadiusControlleverages thesizeprop of its innerUnitControl/InputControlto make its input40px.Testing Instructions
Diff to toggle on 40px height
Screenshots or screencast
Global Styles
Block Editor