Border Controls: Add ability to apply custom CSS classes to popover content

[aaronrobertshaw]

Related:

• Borders: Use new border control components in block support #37770

What?

This provides consumers with a means of applying custom class names to the dropdown popover content in both the BorderControl and BorderBoxControl.

Why?

We need to be able to customize the positioning of the border control popovers in the block editor to match the color control flyouts.

How?

Adds new props to the BorderControl and BorderBoxControl components so that we can plumb through custom CSS classes and pass them to the inner Dropdown components via their contentClassName prop.

As we can't yet leverage the component context system in the block editor this approach avoids adding any hardcoded classes to the components.

Testing Instructions

1. Fire up the Storybook and ensure BorderControl and BorderBoxControl continue to function correctly.
2. Checkout Borders: Use new border control components in block support #37770 which is based on this PR
3. Create a post, add a group block, and select it
4. In the sidebar, experiment with the BorderBoxControl opening each of the border color/style dropdowns (including those in the "unlinked" view). Their width and positioning should be different to the Storybook controls.
5. If still in doubt, inspect a border control popover and confirm the presence of a custom class on the popover.

Screenshots or screencast

[andrewserong]

Nice addition @aaronrobertshaw, the behaviour in the editor makes a great case for adding in the additional classnames. It feels much nicer to have the popover aligned to the editor sidebar rather than covering the border controls:

I just left a (totally optional) suggestion for an additional comment in the readme file, to explain the intended use case, because it took me a minute to work out why we'd need all the additional classnames — though the usage is much clearer when looking at the real world usage in the other PR.

I quite like that this allows styling to be overridden via classnames / CSS, without committing to a particular hard-coded classname (which I know @ciampo has rightly pointed out the risks of in the past). Allowing arbitrarily named, but semantically structured classnames (in terms of the object keys) seems like a good trade-off to me! 👍

[aaronrobertshaw]

Thanks for the review @andrewserong 👍

My understanding is that the ability to make these border control popovers consistent with the color ones in the editor is a requirement to updating our border controls and supporting individual border side configurations.

Given this and that this PR doesn't introduce hardcoded classnames as per previous discussions around new components, I'll merge this now to unblock #37770. I'll take care of any tweaks or follow-ups separately.

[andrewserong]

Given this and that this PR doesn't introduce hardcoded classnames as per previous discussions around new components, I'll merge this now.

Good thinking! And thanks again for splitting out these features in separate PRs, it's made reviewing a couple of them really straightforward 🙂

[ciampo]

Just seeing this now (was AFK at the end of last week), thank you folks for making constant improvements to the components (and for avoiding hardcoded classnames!)

[youknowriad]

I've just seen this addition and to be honest I'm not a big fan. I think it actually forces us to position the popover using a "classname" which might be the only solution right now but AFAIK we want to move away from overriding components using classnames and instead rely on props (well defined behavior).

Maybe a generic popoverProps like we have for Dropdown would have been better here.

[youknowriad]

Context: I'm trying to improve the Popover component in #40740 and this is getting in the way a bit :)

[youknowriad]

I think your use-case here is to position the popovers properly on the side of the sidebar. Here's how I'm solving the same issue for the color popovers using just props 10665a1#diff-ad1d53efac56f53733486d999e58f29074e50b74b692ce5f5fd78b4c3163781cR53-R56

[aaronrobertshaw]

I've just seen this addition and to be honest I'm not a big fan.

This was the best approach I found to meet previous directions around new components. While creating the ToolsPanel I was advised we shouldn't simply expose all props to inner components and instead more deliberately expose only what the individual component needs.

So for these border controls specifically, we didn't want to allow further customization of the popovers beyond that positioning and the class name is (until your refactor I believe) the only way to achieve it.

The repositioning of the popover was a specific request in feedback on the new border controls in order to be able to get them to land and clean up the border support UI which was blocking adoption of that support for a few core blocks.

That is just the history of how we arrived here. The work you are doing on the Popover changes the goalposts a bit so I'm happy to revisit this.

I think it actually forces us to position the popover using a "classname" which might be the only solution right now but AFAIK we want to move away from overriding components using classnames and instead rely on props (well defined behavior).

Thanks for raising the desire to move away from overriding components using classnames.

I'm trying to improve the Popover component in #40740 and this is getting in the way a bit :)

In light of this, I'll create a new PR exposing a popoverProps approach for us to continue the discussion on. Hopefully, that will help unblock your efforts on the Popover.

[aaronrobertshaw]

As promised a PR moving to a popoverProps approach for the border controls is available in #40836

[youknowriad]

Thanks for following up here :) I really appreciate it.