Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add padding to post content in the editor #289

Closed
wants to merge 1 commit into from

Conversation

jffng
Copy link
Collaborator

@jffng jffng commented Dec 9, 2021

Description

This PR adds padding to the title and content in the post editor, so it matches the front-end. It partially addresses #234.

The main issue (aside from the fact that we are inlining the CSS in order to target the .editor-styles-wrapper class) with this approach is that it will apply padding even if you have the Blank template selected, which has no padding by default.

Screenshots

Editor Front-end
Screen Shot 2021-12-09 at 12 21 27 PM Screen Shot 2021-12-09 at 12 21 34 PM

Testing Instructions

  1. Create / edit a post or page that uses the default template. Verify content doesn't touch the edges of the screen.
  2. Verify the padding matches the front-end.
  3. Go to the site editor, verify no padding is applied.

@jffng jffng requested a review from kjellr December 9, 2021 17:32
@jffng jffng linked an issue Dec 9, 2021 that may be closed by this pull request
@kjellr
Copy link
Collaborator

kjellr commented Dec 9, 2021

Thanks. What this does is add some rules to the post/page editor to impose the same left/right padding that the theme adds in its templates right now. The effect is that by default, the appearance matches up in the front-end and the editor. The downside is that nothing inside of post content extends to the screen edges in the editor... but in most cases, that'll match what you're seeing in the front end anyway.

This isn't a good solution, and it feels like a definite workaround. But I'm not sure what other solutions we have at this point, and at least this makes things match.

cc @youknowriad and @jasmussen in case you have other thoughts or ideas. In order to ensure we get adequate testing, I think we'll need to push either this or WordPress/gutenberg#36214 into the next beta.

Before

Note that in the editor, items bump up against the screen edges on smaller screens, and full-width item extend all the way to the screen edges.

Editor Front End
Screen Shot 2021-12-09 at 3 36 02 PM Screen Shot 2021-12-09 at 3 35 03 PM
Editor Front End
Screen Shot 2021-12-09 at 3 38 48 PM Screen Shot 2021-12-09 at 3 39 00 PM

After

Note that the front-end and editor match. But full-width items cannot extend to the screen edges in either case.

Editor Front-End
Screen Shot 2021-12-09 at 3 34 43 PM Screen Shot 2021-12-09 at 3 35 03 PM
Editor Front-End
Screen Shot 2021-12-09 at 3 39 56 PM Screen Shot 2021-12-09 at 3 39 00 PM

@jasmussen
Copy link

Yeah this is a tricky one. I am wondering how the editor should handle this, and I'm not even sure WordPress/gutenberg#36214 would fix it. As far as I can tell, this works for you on the frontend because post content is inside a group that has left/right margin:

Screenshot 2021-12-10 at 10 36 29

But of course the post editor doesn't show this group, and so you have to apply it differently to be wysiwyg. Is that a correct assessment? If yes, then 36214 seems like it would only fix it if it enabled you to remove that group-with-margin wrapper around post content, right?

I personally think it's okay for themes to do this, though I agree it would be nice for much of the complexity to be handled in the block editor when possible. I have some similar rules in my own theme that I look forward to extract.

@kjellr
Copy link
Collaborator

kjellr commented Dec 10, 2021

Yeah, were in a tough spot. Here's the overall situation. At the moment, Twenty Twenty-Two (and most other block themes) are stuck between two non-ideal solutions places:

  1. If the theme uses the current iteration of site padding, its content will work well in both the front-end and the editor. Nothing bumps up against the edges. But the downside to this approach is that nothing can ever go full-width (as noted in 35607). This won't work for Twenty Twenty-Two, because of the full-width black header it needs on top of the default home template.

  2. Alternatively, themes can choose not to use site padding. That's what Twenty Twenty-Two is using right now, since we need the header to be full-width. But in this scenario all blocks bump up against the edges of the screen by default in both the editor and the front end. This looks like a bug to users, (as raised in 35884). To get around this, themes can add padding to the templates themselves, but that doesn't fix anything in the post/page editor. That's where this PR comes in. But it doesn't actually get us to our intended state though, since full-width items in Twenty Twenty-Two's post content still don't extend to the screen edges.

Basically, padding is currently an "all or nothing" approach, and we still don't have a way to let some blocks go full-width, while letting other, non-full blocks have padding. The goal here is really just the status quo: we want to handle full/wide alignments in the post content the same way they've worked for the past three default themes, and countless other 3rd party themes. But it's not possible given today's tools.

At this point, I'm really at a loss for what to do here. But this PR at least syncs things up so that the editor looks like the front end. 🤷

@jasmussen
Copy link

Forgive me if I'm naive, or re-treading past conversations, but I want to be sure the concept is accounted for. On this one:

But the downside to this approach is that nothing can ever go full-width (as noted in 35607)

One of the ideas that spawned the "outer padding" approach was to systematize negative margin CSS hacks, right? I.e. this is what themes used to do:

  • Apply top level padding
  • To blocks that needed to go full width, apply negative margins left and right, that match the padding

If we aren't confident in the systematization of said approach, can we add it directly into the theme instead? I.e. set the global site padding to a custom variable such as --wp--custom--spacing--outer. Then for blocks that are meant to go edge to edge, apply negative margins? I realize it can be hard to make blanket rules. But for example you could have something like:

.wp-site-blocks > * {
	margin-left: calc(-1 * var(--wp--custom--spacing--outer));
	margin-right: calc(-1 * var(--wp--custom--spacing--outer));
}
.wp-site-blocks > .align-full,
.wp-site-blocks > .wp-block-post-content {
	margin-left: initial;
	margin-right: initial;
}

I'm sure there are gotchas here. But to frame it differently: if we permit ourselves a little CSS to handle this — either for the near term or long term if need be — could we write CSS that would handle this for us?

@MaggieCabrera
Copy link
Collaborator

I'm sure there are gotchas here. But to frame it differently: if we permit ourselves a little CSS to handle this — either for the near term or long term if need be — could we write CSS that would handle this for us?

I agree with this. Also having the padding on the templates plus this fix for the editor feels like a worse fix than just having the CSS rules needed for this. We are hoping for this change to make it to GB at some point so we need to think what the upgrade path is going to look like. To me, it feels best to add these simple rules (Blockbase's look pretty similar to what Joen is sharing on the above comment and covers the same cases) and at the point where GB includes a solution for this we can remove the unnecessary css.

@kjellr
Copy link
Collaborator

kjellr commented Dec 10, 2021

By default, the code example above would generally work. The difficulty comes from the flexibility that FSE offers users here. If for example, someone moved the post content into a two-column setup, then that CSS would have no way of knowing and would cause weird overlap.

Hmm. Well actually maybe not actually, since it's targeting direct children. 🤔

@kjellr
Copy link
Collaborator

kjellr commented Dec 10, 2021

An issue though is that this variable doesn't exist at the Gutenberg level: var(--wp--custom--spacing--outer. So if a user adjusted our default padding, or applied padding via Global Styles, then this would break.

Currently Global Styles applies the padding directly inline, so we don't know what the value is.

@MaggieCabrera
Copy link
Collaborator

An issue though is that this variable doesn't exist at the Gutenberg level: var(--wp--custom--spacing--outer. So if a user adjusted our default padding, or applied padding via Global Styles, then this would break.

Currently Global Styles applies the padding directly inline, so we don't know what the value is.

Yeah, so my suggestion here is the same as Blockbase does: we create a variable inside custom, when the GB variable exists, we assign what's in custom to the new variable, then it's backwards compatible, let me spin a PR with it

@jasmussen
Copy link

Yes, my instinct was something along these lines:

	"styles": {
		"spacing": {
			"margin": "0",
			"padding": "var(--wp--custom--spacing--outer)",
			"blockGap": "2.5rem"
		},

The top level padding field isn't surfaced in global styles yet, is it?

Even if it is, it might still be an acceptable tradeoff. We could then consider the approach that TT2 takes here to be a template for how we might incorporate it. From the conversations, it sounds like there are still some inner workings of the "Layout" concept to refine, and I imagine as those pieces fall in place, a clearer path forward will present itself.

@kjellr
Copy link
Collaborator

kjellr commented Dec 10, 2021

The top level padding field isn't surfaced in global styles yet, is it?

It is:

Screen Shot 2021-12-10 at 8 59 36 AM

If it would be helpful (which it sounds like it would be), maybe we can convince a friendly developer to make this value available as a CSS variable for us?

Let's see how @MaggieCabrera's PR works out first though. (Also, thank you both for your help with this! It's a complicated problem!)

@kjellr
Copy link
Collaborator

kjellr commented Dec 14, 2021

Closing in favor of #291.

@jffng jffng closed this Dec 14, 2021
@kjellr kjellr deleted the try/apply-post-editor-padding branch December 14, 2021 20:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post Content padding should match in the editor and front end
4 participants