Size Change: +228 B (0%)

Total Size: 1.31 MB

_{compressed-size-action}

This has been rebased after the changes on trunk to padding.js and spacing-panel.js fixing custom CSS units.

I see this PR has all checks green (including the approval) but I don't see anyone's approval on the reviewers' roster. Did we break GitHub?

Going to give it a shot anyway. Feel free to merge if there was a previous approval. If I find anything worth fixing, we can always prepare follow-ups.

@nosolosw I think it's because it's not targeting trunk yet.

I left a comment at #30608 (comment) Other than that, it looks fine. Though that concern doesn't seem to belong in this PR, as I've just realized this wasn't based on trunk but on #3060 so I should have started with that one.

Should we add the !important styles from my original margin block support PR? I think it makes sense.

Should we add the !important styles from my original margin block support PR? I think it makes sense.

I've added the !important to the layout styles as per your original PR and rebased the PR.

Margins and paddings are so needed, I'm happy to see the feature land.

My only concern is that we are still not really taking a stab at the more generalized efforts in #27331, which see both margins and paddings grouped in a dedicated "Dimensions" panels, such as this:

One aspect there as well, is that margin and padding is a very obtuse control for folks that aren't web-designers, and we might even want to use a segmented control to group the two.

Probably margin is such an important tool that we shouldn't hold it up. But should we at least start to move towards a single "Dimensions" panel?

This is a very welcome PR, thank you for your work!! I tested this out (using #30609) and it worked as intended. +1 on getting this on the group block, it's where we most need it.
I was not able to change the default values for the separator block using theme.json though.

My only concern is that we are still not really taking a stab at the more generalized efforts in #27331, which see both margins and paddings grouped in a dedicated "Dimensions" panels, such as this:

I'd be happy to help make steps towards this happening though unfortunately I'm not very clear on any progress, or the plan to integrate G2 components into Gutenberg as per the screenshot. Similar issues arise with the border support panel refinements requested in #31337.

Probably margin is such an important tool that we shouldn't hold it up.

There are a number of issues blocked by not having the ability to configure margins. Getting this one merged and then iterating on the UI would definitely get my vote.

or the plan to integrate G2 components into Gutenberg as per the screenshot

The designs in #27331 are not reliant on any G2 components being added to the block editor. The system is entirely compoment agnostic, and is purely an expression of which design tools are surfaced in which panels, according to block supports.

The designs in #27331 are not reliant on any G2 components being added to the block editor. The system is entirely compoment agnostic, and is purely an expression of which design tools are surfaced in which panels, according to block supports.

Thanks for the additional info. I'll look into it. Perhaps I was taking the design screenshots too literally.

Perhaps I was taking the design screenshots too literally.

I mean, we'll need some padding, margin, CSS, styling updates, and ideally we have a system that helps float/flex/rearrange/correctly place the form controls based on what's supported and so on, in a 2 or 3 column system. But there are steps that can be taken before that.

I was not able to change the default values for the separator block using theme.json though.

Thanks @MaggieCabrera, you should be able to test this margin support via the theme.json for the separator block now.

In case it helps, here is the theme.json I used while testing.

Thanks @MaggieCabrera, you should be able to test this margin support via the theme.json for the separator block now.

In case it helps, here is the theme.json I used while testing.

Yeah, I was able to see the spacing menu on the editor, what I meant was that I couldn't do something like this on my theme.json file:

			"core/separator": {
				"spacing": {
					"margin": {
						"top": "200px"
					}
				}
			},

This is going to be important for certain blocks so that the theme can set some good default values and we can actually do it for the padding right now.

@MaggieCabrera sorry if I wasn't clear in my comment. I meant you could now do exactly what you were expecting in setting the margins within the theme.json styles. The linked gist actually set the top and bottom margins.

	"styles": {
		"blocks": {
			"core/separator": {
				"spacing": {
					"margin": {
						"top": "100px",
						"bottom": "100px"
					}
				}
			}
		}
	}

Please let me know if there are any other issues you were facing.

@MaggieCabrera sorry if I wasn't clear in my comment. I meant you could now do exactly what you were expecting in setting the margins within the theme.json styles. The linked gist actually set the top and bottom margins.

Yeah you are totally right, it was a theme issue not letting me see the changes. I tested again with your theme.json file using emptytheme and I can see the margins on the frontend but the editor shows 15px margin on the separators isntead of 100px:

Editor	Frontend

I can see the margins on the frontend but the editor shows 15px margin on the separators instead of 100px:

There was a 15px default value applied to the separator block specifically that was overriding the theme value. That was removed in 0f57553 before I advised it should be right to test again.

Can you confirm if you pulled the latest changes from #30609 before re-testing?

This is what I see when loading the editor with the theme.json gist linked above, no defaults in the control.

It's also worth mentioning there isn't a means at present to determine the default merged ( default, theme & user ) style value to populate the block support controls within the block editor, only the full site editor.

If we wish to address that, it's an issue for all current block support controls and not unique to this PR. A separate PR would be best to discuss that further if desired.

Can you confirm if you pulled the latest changes from #30609 before re-testing?

Yes, I pulled the latest from the branch and it works nicely, awesome!! Thanks for your help and for the PR, can't wait to be able to play this :D

I think the dimensions UI redesign can be considered a follow-up.

I've rebased again to resolve a conflict. Should be ok to merge once tests pass. Then we can iterate on the dimensions UI redesign as suggested.