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

Removed !important from alignment margins. #32213

Closed
wants to merge 1 commit into from

Conversation

pbking
Copy link
Contributor

@pbking pbking commented May 25, 2021

Description

So that exceptions can be coded for blocks and elements inside of a container controlling layout !important was removed from the margin styles being applied.

These !important attributes were introduced in this change and described here.

...if you apply a margin to a block, it overrides the "margin: auto" applied by the alignment (layout), the only way to solve that is to use !important for these. ...

However this appears to not (or no longer) be the case and !important can be removed.

How has this been tested?

Leveraging emptytheme apply theme.json similar to the below where a block defines both TOP and LEFT margin styles:

{
	"version": 1,
	"settings": {
		"spacing": {
			"customPadding": true,
			"customMargins": true
		},
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"blocks": {
			"core/paragraph": {
				"spacing": {
					"margin": {
						"left": "100px",
						"top": "100px"
					}
				}
			}
		}
	}
}

I used a simple test page with the following block markup:

<!-- wp:group {"align":"full","backgroundColor":"blue","layout":{"inherit":true}} -->
<div class="wp-block-group alignfull has-blue-background-color has-background"><!-- wp:paragraph -->
<p>This is a group with a paragraph in it.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

Note that both BEFORE and AFTER the change the paragraph adopted the TOP margin style expressed in theme.json (100px) and the LEFT margin style from the alignment class (auto).

This was also tested in the same manner with various other blocks (group, columns, image)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

So that exceptions can be coded for blocks and elements inside of a
container controlling layout !important was removed from the margin
styles being applied.
@youknowriad
Copy link
Contributor

Here's what happens with this PR:

I have a block that supports margin (we don't have any at the moment but you can add "margin": true to the "spacing" section of any block.json to try it), and add a random 100px margin to that block, notice the margin applies to the top and bottom properly but it overrides the "margin: auto" used for alignments (the block is supposed to be centered inside the content size area), which means the block becomes wider.

@scruffian
Copy link
Contributor

I see the problem @youknowriad raises, but I don't think that the !important declarations are a good solution because if you add a left margin then it doesn't do anything!

I'm going to try to come up with a different way of centering the content inside containers so that margins work as expected...

@scruffian
Copy link
Contributor

I think this exposes a flaw in the way that alignment styles work. Here's an alternative approach which I hope overcomes some of the issues raised: #32231

@aristath
Copy link
Member

With #32231 merged I believe we can close this one?

@scruffian
Copy link
Contributor

I agree

@scruffian scruffian closed this May 27, 2021
@scruffian scruffian deleted the fix/remove-important-margins branch May 27, 2021 12:03
@pbking pbking restored the fix/remove-important-margins branch May 28, 2021 14:27
@pbking
Copy link
Contributor Author

pbking commented May 28, 2021

While I understand the situation raised by @youknowriad isn't solved by this branch I argue that likewise it shouldn't be solved with an !important. While we don't know what the solution will be yet I suggest we re-evaluate this change as a solution that allows themes to move forward. Preventing a theme from expressing the style of "alignfull" seems like a larger problem than inappropriate margins when nothing currently allows for margin configuration yet anyway.

@pbking
Copy link
Contributor Author

pbking commented May 28, 2021

FWIW a temporary situation has been figured out using !important in the theme to override the !important from Gutenberg. Keeping this closed is fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants