Borders: Add new BorderControl component #37769
Conversation
aaronrobertshaw
added
[Status] In Progress
|
Size Change: +8.79 kB (+1%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/combined-border-controls
branch
from
776891f
to
a650d77
Compare
|
@shaunandrews I think I have the first draft of the new border and border-box controls ready for some feedback if you have the time to take a look. I've whittled it down to where there are only a couple of things left that would be good to get design feedback on. These include:
|
aaronrobertshaw
force-pushed
the
try/combined-border-controls
branch
3 times, most recently
from
5d067c2
to
7be6d5d
Compare
|
I've moved this PR out of the draft status. It will still have plenty of areas to improve but is ready for an initial round of feedback. |
aaronrobertshaw
changed the title
|
Thank you @aaronrobertshaw for working on this PR, it's a great amount of work! Before I go into a deeper review, I wanted to make a couple of higher-level considerations. The first one is that these components look very tailored to the needs of the editor (ie. setting borders) — I wonder if other packages (like I know that we already have a few components in The other main consideration is regarding the size of this PR. I read the disclaimer at the top, and I understand that this PR exists mainly for ease of testing / managing changes. But, when reviewing these PRs in details, I'd prefer if we split them in smaller, iterative PRs. To start, we could have a few small independent PRs:
At this point, we are going to be able to focus on What do you think? |
|
Thanks for the initial feedback @ciampo 👍
I've discussed this with the team again today and the consensus was that the components package was the best home for these components. Some of the reasoning for this includes:
As well as initial development. If splitting this PR into 5 PRs as suggested occurs from the outset and we also then have the PRs exploring the use of these components for our end-goal purposes (which itself could be split) we get an unsustainable chain of PRs where excessive effort is spent switching branches to make minor edits forcing repeated rebases and increasing the likelihood of things being missed. I feel that effort is better spent refining the components and then once more stable, easily splitting the PR into reviewable chunks. The intention is very much to get a general OK that these new components faithfully bring Shaun's design to life, their handling of mixed border values is suitable, and the general UX is improved. Then split up this large PR so it will be easy to review. Hence the disclaimer mentioned.
While the above occurs, I absolutely can begin splitting out the changes that aren't likely to change as you've suggested 👍 My plan is to have most of that done this afternoon.
My guess is no other typed components were using them and it wasn't included in the
I'm not sure this is truly "independent" but if it is satisfactory to simply note on the separate PR why these ignore checks are being added (so the components can be used in other typed components under development), then that's another easy one to split off. It might actually need to be a part of the first suggested PR as adding the ColorPicker to the tsconfig.json will require its dependencies to be included as well. At least that was my experience while attempting to resolve the typing errors during development of these components.
In summary, I think we keep the new components in the components package. I will split out the basic typing changes/fixes into separate PRs now and separate the Thanks again for looking at this one! 🙂 |
There was a problem hiding this comment.
Fantastic work here @aaronrobertshaw! This is testing really nicely for me in Storybook 👍
I noticed a couple of tiny design things that we might want to tweak slightly. Hope you don't mind some detailed early testing notes! 😀
In the design in #35602, it looks like there's a little more horizontal spacing surrounding the slider to give it some breathing room. Here the focused state has the circle bump up against the border of the input control:
While clicking on the BorderDropdown component, if the color is currently empty the cross disappears in that active/target state. If you click quickly it effectively looks like it flickers a little when you click on it. Not sure if that's an issue? It also looks like there's a border radius on the grey colour, so it doesn't go to the full edges of the component.
Also in the above screenshot, with no colour selected, the border renders as a thick black border in the preview there. If someone's designing a border style where, say, they have a thick bright blue top / bottom border but want the left / right borders to be transparent, is there a more inconspicuous way that we can visually represent transparency?
Finally, in the known issues you mentioned the tooltip position and layout. It looks like the tooltips are showing immediately on clicking the BorderDropdown for me. Is that what you meant? In the below screengrab, I can see a few issues:
- The tooltips show immediately on clicking the component
- The tooltip position then moves to the right of the popover
- It takes a little while for the close button icon to render in the correct position and size
In terms of the size of this PR, I'd actually be in favour of keeping much of it together in the one PR while you're iterating on it. It's really nice being able to test it all at once, and personally I don't mind the big changeset in terms of added files, given that there's a consistency to the conventions used, and many of the added or updated files are either quite small changes or readme additions.
Happy to do further testing / review if and when it helps!
|
I've created separate PRs for the minor typing error fixes and
As these get merged, I'll rebase this accordingly. |
I always appreciate your detailed reviews @andrewserong. They are welcome any time 🙂
The difference here I believe is the focused state of the slider getting that extra ring around the slider's "dot" handle. I tried to stay true to the design however I didn't have a pixel-perfect design reference handy but that should be easily changed when we get some further design feedback. It might also be worth noting that I put this alongside the border-radius control and made them consistent in terms of spacing between the input and the slider. If we change one to add breathing room I'd suggest we tweak the other too.
This wasn't intended and is something I can definitely polish. Thanks!
This is because it should be rendering with the browser's default border color I believe,
If we have
The close button's tooltip is currently showing as the popover opens. Because the button has a flex layout the sibling
Sounds like a plan until this is ready for final review.
Thanks, they'll be plenty of testing and reviews to come 🙇 |
Thank you for looking into this, and being super available in creating smaller PRs. I will focus on reviewing those first, and will then come back to this PR for a more detailed look. |
aaronrobertshaw
force-pushed
the
try/combined-border-controls
branch
from
455da67
to
7552f9d
Compare
My apologies for being unclear, I'm using "flyout" as a fancy term for a popover that opens leftward instead of downward: The designs in #35602 do share a little DNA with ItemGroup items in terms of being row-based, especially the combined border-swatch/border-width pill. So I'm simply suggesting the border controls panel opens leftwards in the same way as itemgroups do. |
I think this is more a case of me being a bit slow after some AFK. Thanks for the clarification 🙂
I'm working through some refinements for both this and the |
|
This is an extremely badly stitched together mockup cobbled together of bits and pieces, just to illustrate what I mean:
Key here being that it'd be nice to reuse the same interface we use for colors, here. |
The current popover for the The demo of the Am I correct that the points of difference between the above mockup and what we have currently are:
Or, are these differences (other than the positioning) just part of the fact it was a quickly stitched together mockup? Positioning of the flyout I'll make this change once I'm clear on what other changes are needed for the flyout. Removal of "Border color" heading and close button I'm no designer but it feels like removing these makes the style options look even more like an afterthought, jammed into that flyout. Any ideas on improving the delineation there between the color palette and styles? Removal of the "Reset to defaults" button We need this button to facilitate clearing custom colors without undoing width selections as well. Otherwise, after selecting a custom color you need to click a named color, then click it a second time to actually clear the border color. |
|
There are definitely limitations because of it being stitched together like this! So don't take this as a major shift. It's almost certainly fine to land this one as-is. I'm mainly looking a bit forward, and it would be nice to unify as many of these disparate interfaces, as makes sense.
Behaving the same as the color flyouts, by being placed to the left of a toggled-state ItemGroup item, the flyout would have context from its "opener". The panel itself by virtue of not having to fit within the 280px limitations of the inspector, would also afford both a wider and a taller canvas to put controls. If you need to put the style controls at the top, that would be an option too. Yes, we definitely need better palette and color swatch management — also a separate task to this PR — right now if a theme registers too many colors, the interface becomes unwieldy. In terms of leveraging the flyout, for context and canvas size benefits as mentioned above, #39427 details similar changes to the cover block, reducing the prominence of the focal point picker but still making it conveniently available in context of what you're doing. #39452 similarly does so for duotones.
I don't think we'd be able to do this unless we made bigger changes to the control, whether that be ToolsPanel integration, or otherwise. I didn't intentionally omit that from the cobbled-together mockup! |
This passes through the __experimentalIsRenderedInSidebar prop to the border control dropdown which can then use that to determine the dropdown's position.
Given a little uncertainty around how we want the color/style dropdown to look, I've just added a prop to toggle its display for now. This is included in the Storybook example controls so it can be evaluated. Along those same lines, I've added the ability to toggle multi-origin colors as we have in the block/site editors presently.
I'm still looking into this issue. It appears that in the storybook example, the tooltip popovers are being rendered within the dropdown's popover. That nesting might be the root cause. |
|
Now the issue with the misaligned tooltips has been resolved (535cd0d) I think this is in a position that we can merge it and make any tweaks via followups. My only question is whether the recent addition of a prop to toggle display of the color/style dropdown header is acceptable. Merging this will help unblock the PR creating the @ciampo, @mirka, and @jasmussen do you have any thoughts or objections to landing this one? |
|
Taking another look! Here's what we have today: That's a biggish panel for borders, with lots of atomic controls. Here's a GIF of the new border panel:
I like that there's DNA shared with
That is, each I still think we should move towards a flyout that opens leftward, just like they do from Can we use 50% of the column for the border unit and 50% for the slider? Right now it gets a bit compressed with high values and long units: Tying it all together, you could end up with something like this:
The larger footprint would afford the 16px padding left and right in the
Does all that have to happen in this PR? No, but it's an idea for where we could take this in the future, so as long as the current PR doesn't paint ourselves into a corner with regards to getting there, seems fine to land! Nice work. |
|
Thanks for the thoughtful feedback @jasmussen 👍
The dropdown for this component gets given the same position as the flyouts you are referring to. Within the storybook though the component doesn't get the same treatment as it would within the context of the block editor so I don't think we should get too hung up on that at this stage. I will address these concerns while refining #37770 and thus before it lands in the block or site editors.
The Another thing to consider here is this component (
Hopefully, the answers above show we shouldn't be painted into a corner landing this. I'll wait for green lights from Marco and Lena before I pull the trigger. |
🚀 — my bad!
Are we sure it's a good idea to allow this width? I would expect the whole component, including the slider, to be always the full width of its container and scale as best it can using built-in min/max values inside. It doesn't seem like there's a tremendous value in allowing arbitrary widths of the inner pieces of the component. Right now we have a great deal of vertical misalignment going on in panels: Even though #27331 is closed now, that's one of the primary efforts that was meant to be addressed. Components should size themselves smartly according to what was shown and hidden, so that things would line up across. Quick and dirty doodle:
As shown in the block-spacing/padding mockups above, this can still work by letting the link/unlink button be part of the 50% space occupied by the slider: That's all to say: if we can rely on intrinsic behaviors for each of these ingredients, hopefully it can put is in a much better place. What do you think? |
|
Thanks for the extra thoughts and explanations @jasmussen 👍
Given we are creating a package of components that could be used to build any UI, I'd say the flexibility is desirable. I wonder if we are focussing too much on how the component will be used, shown or configured in our editors at the moment. This PR and the
The
At present the I'm happy to work on refining the default widths/sizing in a follow-up. |
Given the existing size and length running of this PR I think merging it based on the approval and then adding some follow-ups for the width/sizing issues is a good approach. |
Thanks for the sanity check. I'll get this merged. 🚢 I think we can use the PR updating the border block support to focus the discussion around sizing and vertical alignments and inform it via that real-world use case. |
Related:
Description
This PR adds a new
BorderControlcomponent aimed at improving the UX around configuring borders.BorderControl{ color: '#000000', style: 'solid', width: '0.5em' }Note
The PR began life introducing two new components,
BorderControlandBorderBoxControl. They have now been separated into separate PRs. Some of the implementation decisions in this were driven by the component's bigger picture use as part of theBorderBoxControlHow has this been tested?
Via unit tests;
npm run test-unit packages/components/src/border-control/test/index.jsManually:
npm run storybook:devBorderControlScreenshots
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).