WordPress / twentytwentytwo

Twenty Twenty-Two, the default WordPress theme that will launch with WordPress 5.9.
405 stars 92 forks source link

Add padding to post content in the editor #289

Closed jffng closed 2 years ago

jffng commented 2 years ago

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.
kjellr commented 2 years ago

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 https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

Yeah this is a tricky one. I am wondering how the editor should handle this, and I'm not even sure https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Closing in favor of https://github.com/WordPress/twentytwentytwo/pull/291.