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

Move the root body margin reset into the Core theme.json #36996

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 30, 2021

Description

By popular request this PR explores an alternative fix for #36147 which moves the CSS reset into the root theme.json file in Core.

As I see it this is inferior to my preferred solution in #36960 for the following reasons:

Riad's reason

See #36996 (comment).

root margin value will be overwritten

the root margin value will be overwritten in its entirety by anything provided by the Theme's theme.json. For example try setting this in your theme.json:

{
    "styles": {
        "spacing": {
            "margin": {
                "left": "0",
                "top": "100px"
            },
            "padding": "0"
        }
    }
}

This will result in the following CSS being generated which causes us to lose the 0 margin on the bottom and right sides:

Screen Shot 2021-11-30 at 11 48 08

Reduced clarity of intent

The reason for including body { margin: 0; } is as a CSS reset for browser defaults. That's why it has a specific if() conditional in the code. By moving this to the Core theme.json we risk this seeming like just another default value provided by Core when in fact it has a very specific intent. I believe the intent will be lost especially when we consider you cannot add comments to .json files.

Potential backwards compatiblity issues

Theme have come to expect body { margin: 0 }. As explained above if we include this in Core theme.json then if the user provides any margin value in their theme.json then this risks the margin: 0 being lost. That could cause many themes to suffer visual regressions. Granted this is simple to solve but we want to avoid these kind of rippling changes, especially this close to 5.9.

Closes #36147

How has this been tested?

Follow same instructions as #36958.

Please ensure you test both the string and object based margin properties in your theme.json as they will produce different results.

Screenshots

Types of changes

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).

@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

cc @overclokk - this is the approach you've been advocating for.

@getdave getdave added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 30, 2021
@getdave getdave self-assigned this Nov 30, 2021
@getdave getdave marked this pull request as ready for review November 30, 2021 11:57
@ntsekouras
Copy link
Contributor

ntsekouras commented Nov 30, 2021

This will result in the following CSS being generated which causes us to lose the 0 margin on the bottom and right sides:

Yes, this won't work as expected in cases like this: "margin": { "top": "10px" }

It will still get the browser defaults for the undeclared sides.

--edit

Actually this can work by declaring the default margin like this: "margin": { "top" : "0", "left": "0", "right": "0", "bottom": "0" } }.

I like Dave's reasoning about Reduced clarity of intent, but no strong opinions here after all 😄

@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

Actually this can work by declaring the default margin like this: "margin": { "top" : "0", "left": "0", "right": "0", "bottom": "0" } }.

To confirm I tried this and it does work so it's an option.

@overclokk
Copy link

Actually this can work by declaring the default margin like this: "margin": { "top" : "0", "left": "0", "right": "0", "bottom": "0" } }.

I think this is the option that works better because in a theme I can override also a single position or all positions.

If I wan to override single position:

Theme theme.json

	"styles": {
		"spacing": {
			"margin": {
				"top": "5px",
				"bottom": "5px",
			}
		}
	}

Ans the result will be:

body {
	margin-top: 5px;
	margin-right: 0;
	margin-bottom: 5px;
	margin-left: 0;
}

And if in a theme I want to override all I can do like this:

	"styles": {
		"spacing": {
			"margin": "0"
		}
	}

And the result will be:

body {
	margin: 0;
}

@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

I updated the PR to use the object version which allows a more flexible override via the user's theme.json file.

@youknowriad
Copy link
Contributor

I think conceptually I prefer the other approach because while margin (from theme.json) of the root element is applied to body right now, it might not be always the case in the future, because the actual root there is .wp-site-blocks and the margin: 0 needs to specifically target body to reset the browser defaults.

@overclokk
Copy link

I think conceptually I prefer the other approach because while margin (from theme.json) of the root element is applied to body right now, it might not be always the case in the future, because the actual root there is .wp-site-blocks and the margin: 0 needs to specifically target body to reset the browser defaults.

Adding if statement you always add potentially bugs, this should be handled by the system in place, the theme.json, this also avoids misunderstandings of where the code is by devs, so if you need to reset the body margin (for any reason) why not add a null value and when the file is parsed just remove margin value if you need it?

So, if I want to remove enterely the margin I can add this to my theme.json (Now it does not work obviously)

	"styles": {
		"spacing": {
			"margin": null
		}
	}

So this will be handled by the parser and not hardcoded.

@getdave getdave changed the title Make the root body margin reset into the Core theme.json Move the root body margin reset into the Core theme.json Dec 1, 2021
@getdave
Copy link
Contributor Author

getdave commented Dec 1, 2021

I agree that it is perhaps a little confusing that a browser CSS "reset" for the body is added via the same mechanism that handles Global Styles. This makes it appear as though it is derived from the theme.json even though it is not. We should look to improve that...but...

For this Beta period, we need to focus on fixing bugs and I fear exploring the route in this PR further may lead us into "feature development" territory.

I'm not saying that we shouldn't explore this in the future (i.e. WP 6.0 cycle) but for the purposes of immediately resolving the Issue originally reported, we should aim for the simplest solution with the fewest knock on effects.

The majority of contributors seem agreed that the best solution to achieve that is now #36960. So let's proceed with that.

Thank you for advocating for alternative routes. We should/could raise Issues to explore this further in the future. 🙇

@getdave getdave closed this Dec 1, 2021
@youknowriad youknowriad deleted the fix/body-margin-reset-via-inclusion-in-core-root-theme-json branch December 1, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The margin added at the top level styles (for body) in theme.json is override by a margin added from gutenberg
4 participants