Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ellatrix opened this issue Jul 19, 2022 · 5 comments
Open

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

ellatrix opened this issue Jul 19, 2022 · 5 comments
Assignees
Labels
[Block] List Affects the List Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@ellatrix
Copy link
Member

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.

@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended [Block] List Affects the List Block labels Jul 19, 2022
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 20, 2022
@oandregal
Copy link
Member

oandregal commented Aug 4, 2022

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 #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
Copy link
Member

ramonjd commented Aug 4, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

TLDR. I understand we can close this issue because:

  • Spacing is working fine for list v2 but in TwentyTwentyOne. It should be addressed in the theme.
  • If a theme uses theme.json to set the global block gap style it doesn't affect the items in the new list v2, so we don't need to do anything.

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:

  • Using TwentyTwentyTwo theme.
  • Enable layout in the list v2 block by setting supports.spacing.blockGap to true. This had the result of showing the UI for dimensions and the block control. Using that control to set a blockgap didn't have any effect.
  • I also tried tweaking the spacing by adding to the theme.json of the theme a value to styles.blocks.core/list.spacing.blockGap. It didn't have any effect either.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] List Affects the List Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants