WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.32k stars 4.12k forks source link

List v2: default blocks styles (margin) applied to list items #42526

Open ellatrix opened 2 years ago

ellatrix commented 2 years ago

We currently have the following CSS applied to all blocks:

.editor-styles-wrapper [data-block] {
    margin-top: var(--global--spacing-vertical);
    margin-bottom: var(--global--spacing-vertical);
}

But for list items this isn't a good default. I don't think this generally is a good default to have either. Themes should define margins, not us. I remember discussing this with @jasmussen once upon a time.

oandregal commented 2 years ago

The --global--spacing-vertical is something TwentyTwentyOne specific and it only affects the editor, not the front (because the theme checks for the existence of the [data-block] attribute, which is not present in the front).

Though Matías brough up a related question we need to look at: what happens to the new list block on a theme with a vanilla theme.json if you modify block gap at the global styles level? It sounds like for lists core (the theme.json bundled in wp-includes) may want to introduce a default for block gap specific to this block. Perhaps also for quote https://github.com/WordPress/gutenberg/pull/25892

Something like the following should suffice:

{
  "styles": {
    "blocks": {
      "core/list": {
        "spacing": {
          "blockGap": "0"
        }
      }
    }
  }
}

I need to check how block gap / layout works more in depth to know better. cc @andrewserong @ramonjd @aaronrobertshaw in case you can offer any thoughts.

ramonjd commented 2 years ago

Themes should define margins, not us.

I can only see this at work in TwentyTwentyOne's editor CSS.

So isn't this a case of the theme defining margins?

what happens to the new list block on a theme with a vanilla theme.json if you modify block gap at the global styles level?

@oandregal I tested adding the following to TwentyTwentyOne's's theme.json

            "core/list": {
                "spacing": {
                    "margin": {
                        "top" : "0",
                        "bottom": "0"
                    }
                }
            }

This will add CSS to reset margins for ul elements, but it's not specific enough to override TwentyTwentyOne's CSS.

I'm pretty sure blockGap won't be applied to this particular block because it hasn't opted into layout support.

Generally speaking, the logic for outputting block gap styles at the global level is in the Theme_JSON class. For block supports (that is, blockGap values in a block's style attributes) it's in layout.php.

@andrewserong for a fact check 😄

andrewserong commented 2 years ago

I'm pretty sure blockGap won't be applied to this particular block because it hasn't opted into layout support.

That's correct — to enable spacing between li elements, you'd probably need to opt-in to the Layout support, and then the blockGap value could be set either at the individual level in the editor, or at the block level in global styles, with a fallback to the root blockGap in styles.spacing.blockGap in theme.json. With the list block, that default is probably too much spacing, so perhaps the base theme.json in Gutenberg could set styles.blocks.core/list.spacing.blockGap to a default of 0 to have the desired default of 0 spacing between list items, or something like that?

andrewserong commented 2 years ago

Another thing to keep in mind, is that flow-based (default) Layout gap styles are not output in Classic themes, or blocks-based themes that have not opted-in to blockGap. So depending on how the block is being built, the simplest approach might be for the block to ship with some opinionated CSS?

oandregal commented 2 years ago

TLDR. I understand we can close this issue because:


Longer context. This is what I did:

Using TwentyTwentyTwo, I set styles.spacing.blockGap to 5rem (it is 1.5rem now) and the list items for the list v2 are unaffected (maintain the same spacing as before):

Captura de ecrã de 2022-08-05 11-29-05

A second experiment I ran was: how can I intentionally control the spacing between the list items? Guided by your directions, this is what I did:

I'm probably missing things to make this work, as I haven't really dug into the layout work. In any case, controlling the spacing between list items is out of the scope for list v2 and can be looked at after, if it's something we want to implement.