canonical / vanilla-framework

From community websites to web applications, this CSS framework will help you achieve a consistent look and feel.
https://vanillaframework.io
GNU Lesser General Public License v3.0
841 stars 167 forks source link

Spacing below tables and divs #2420

Closed nottrobin closed 5 years ago

nottrobin commented 5 years ago

@barrymcgee discovered a couple of spacing issues in the MAAS docs work.

I fixed both issues in the local project, but I think maybe we should consider whether Vanilla core should have taken care of them.

In the case of the tables, where an <h2> directly following a <table> has no space between the two, the issue was because the spacing that would normally appear above headings comes from this logic (as in, explicitly only apples when h2s follow p elements):

p:not([class*="p-heading--"]):not([class*="p-muted-heading"]) + h2,
p:not([class*="p-heading--"]):not([class*="p-muted-heading"]) + .p-heading--two {
    padding-top: 1.6rem;
}

This could be fixed by simply adding table into the selectors for the above rule, but it seems like we might be chasing our tail with that solution because there are almost certainly other basic elements that would cause the same problem. It would seem to me that we'd be better off with a catch-all rule of some sort, like h2:not(:first-child) {padding-top: 1.6rem;} which we then override for any special cases we need (e.g. p[class*="p-heading--"][class*="p-muted-heading"] + h2 {padding-top: 0}). Of course I don't know the history here.

If we did find a more generic version of the solution then it could solve the second problem I had, which was about a p-notification following a standard div.

For your consideration.

deadlight commented 5 years ago

@lyubomir-popov I see that you've assigned yourself. If you want to work on it, put it in the vanilla squad's small tasks epic.

lyubomir-popov commented 5 years ago

@deadlight yes I have some ideas about this, but I'm not sure the small tasks epic is the correct place for it. It will probably involve some discussion and a strategy for how to apply in many similar scenarios. I'll do some investigation first and write down some notes, then maybe discuss in the WG?

lyubomir-popov commented 5 years ago

@nottrobin @barrymcgee @deadlight I want to make a distinction between tables and divs, because they are examples of very different issues:

  1. Tables In the case of the table, it's a simple bug - tables should be spaced out using the standard amount of white space between big elements ($spv-outer--scaleable). It is actually there, applied as padding-bottom, but it doesn't work because of border-collapse. I'll put in a pr to fix that.

  2. Divs Using generic tags in selectors is discouraged in vanilla, (not sure if it is written down anywhere, but it's a sure way to get a -1 from @anthonydillon :) ), and I think with good reason. It invariably backfires, like the infamous * + * {margin-top: 2rem} that we took out in v1.7.

AFAIK there is no safe way to handle things like this without adding a class somewhere in the markup, like .p-strip.is-shallow. I appreciate the fact that this might be easier said than done in automatically generated content, but if that's the case, then wouldn't a local rule be much safer than adding this to the framework?

Specifically about the suggestions mentioned above:

p+h* is a series of special rules that fine-tune typographic spacing. Extending this to arbitrary elements is not viable, because unlike headings, the list of elements can be endless.

h2:not(:first-child) is tempting, but it would backfire in cases like <hr><h2>...</h2>, where you might want the line to be close to the heading.

nottrobin commented 5 years ago

Okay, I can certainly see the argument that in this case, where I can't add classes to the markup, it makes sense that I might have to write a few local style rules. That is not a problem, and what I've done, and if it's decided that this case can't be covered by Vanilla, that's fine. But, this example aside, let's consider what would be ideal for Vanilla to do - regardless of what might or might not get a -1 or +1 from @anthonydillon,

I think Vanilla should be written in as straightforward a way as possible, to increase the chance of handling as many edge-cases as possible.

For this reason, I think it would be vastly preferably to have a layout system which got as close as possible to defining consistent empirical spacing between elements on the actual element themselves, regardless of context. For the fundemental spacing above a heading to rely on the type of the block element before it seems like rather fragile logic, and relies on us having successfully predicted all the possible combinations of markup flow, which we probably haven't. (The reason things like border-collapse exist is precisely to help you do this).

As you point out, this fragility can be managed, for our own team, by -1ing PRs that don't conform with the framework's view of what order markup should appear in, but it doesn't seem to make for a great basis for a community framework.

So I think examples like my </div><p class="p-notification"> one should help act as signals that Vanilla might not be as solid as it could be in its basic logic. I'm really just suggesting that Vanilla try to handle edge-cases as well as default browser styles - e.g. in Firefox:

image

lyubomir-popov commented 5 years ago

@nottrobin @anthonydillon @barrymcgee

For this reason, I think it would be vastly preferably to have a layout system which got as close as possible to defining consistent empirical spacing between elements on the actual element themselves, regardless of context.

This is how it works 99% of the time. Importantly though, spacing is defined at the bottom of elements, not at the top - this is due to following a best practice called "single direction margin declarations", and we've chosen this direction to be bottom for various reasons.

So spacing above the element is consistently dealt with by the preceding element, or a container like .p-strip. Any individual element only cares about the spacing underneath it, with p+h* being a special case of fine-tunings reserved for a very limited set of text-only elements.

There is some spacing above headings, the padding-top nudge that gets them to sit on the closest baseline. It is deliberately minimal, and this is to achieve hack-free alignment of different text and ui elements sitting next to each other. A couple of examples of where this is necessary:

Working example: https://codepen.io/lyubomir-popov/pen/WVxJdy I've inlined explanations of the current strategy in the codepen, please take a look.

In summary, it's the preceding element's job to clear enough space after itself. This is done everywhere we can target an element with a class, or a tag that is specific enough not to require frequent overrides, like input, fieldset, hr, pre, ul, ol, figure, and table (pending the table bug fix).

All these, and any other blocky elements should use the consistent $spv-outer--scaleable. Examples of patterns that use the same are: .p-matrix, .p-media-object, .p-card, etc.

Would it be useful to expose this as a class? So you can apply the standard margin-bottom to generic elements? I've added what this might look like at the bottom of the codepen.

lyubomir-popov commented 5 years ago

@anthonydillon did you get a chance to think about this? If you remember the conversation about margin vs transparent border vs display: table-footer-group

anthonydillon commented 5 years ago

Fix landed in #2559