Change BaseControl sibling selector to use margin-top

[chrisvanpatten]

Description

Changes the BaseControl sibling selector to use margin-top instead of margin-bottom for consistent spacing. Fixes #17984.

How has this been tested?

Confirmed in CSS via web inspector and in local development environment.

Screenshots

Before:

After:

Types of changes

Small CSS bug fix.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[youknowriad]

I haven't tested but this makes sense.

[jasmussen]

In principle, this seems fine.

However the sad thing with all small changes is, they're never small. This happens:

I imagine that some plugins rely on the current configuration as well, so this will probably need a dev note too.

Thank you for the PR! 🤟

[ItsJonQ]

👋 Hallooo!! Just checked out this PR.

In principle, this seems fine... However the sad thing with all small changes is, they're never small.

@jasmussen That's been my experience as well.

Assumed margin (top or bottom) via DOM order is normally okay for simpler use-cases. By simpler use cases, I mean when most sibling DOM element rendering is consistent or has only a handful of variation. For example, items that appear within a blog post.

However, for more app-like UI, it's very difficult to predict how/when elements may need to render above, below, or next to each other, with layout rules defined by display: flex, float, position, or anything in between.

@chrisvanpatten I really appreciate your intention + initiative here.

Changes the BaseControl sibling selector to use margin-top instead of margin-bottom for consistent spacing.

If I may, by "consistent spacing", I feel like you don't necessarily mean specific px values, but rather... the darn thing should look/work the same. Every time. So we never have to think about it again 😂

This is a long shot, but I think a good solution would be for margins to be defined explicitly - either by using a layout-based Component or ad-hoc - both using a predefined set of layout/spacing values (e.g. 4px, 8px, etc...). Of course, this would involve removing the margins for BaseControl, which will most likely also break a bunch of things 😛 .

---

As for this PR, I'm not sure how to proceed, as changing the margin from bottom to top solves some use cases, but has unintended side effects for others.

[chrisvanpatten]

@ItsJonQ Thanks for taking a look!

If I may, by "consistent spacing", I feel like you don't necessarily mean specific px values, but rather... the darn thing should look/work the same.

Yes exactly. I’m less concerned with either actual size/value, and more concerned that spacing is treated consistently.

That said, I’ve obviously let this grow stale, and frankly that’s because I haven’t been totally confident in my solution for all the reasons you hit upon. It solves some cases but risks introducing other regressions. I also agree with what you hinted at — this is probably best addressed in a more holistic way, ideally at the component level.

So with that in mind I think I’ll close this PR, although I’m happy to also open an issue for discussion of this problem at a higher level. And if this solution seems like a viable path at a later point, we can always reopen or revisit.

[chrisvanpatten]

I will also say, for posterity, that I believe the current behaviour is a mistake. Not a mistake of the “I disagree with this” variety, but I actually think it’s a genuine bug, introduced unintentionally.

Unfortunately, it’s a bug that we’ve come to depend on in certain circumstances. As a result, it’s tricky to fix, because the solution inevitably breaks the things which assumed the bug was intended behaviour 😅