search

WordPress / twentytwentytwo

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

Try: add padding CSS rules instead of in the templates #291

Closed MaggieCabrera closed 2 years ago

MaggieCabrera commented 2 years ago

This PR provides an interim solution to the padding problem that eventually will be solved in Gutenberg (https://github.com/WordPress/gutenberg/issues/35607). This leverages a custom outer spacing variable that will be absorbed with the future variable implemented for the editor plus a series of CSS rules that will eventually disappear as the editor provides a global solution instead.

To test this check that the full width blocks all over the theme go all the way to the borders of the viewport while the rest of the site maintains a padding on mobile to stop content from extending. The editor should show the same as the frontend does.

Alternative to https://github.com/WordPress/twentytwentytwo/pull/289

Addresses https://github.com/WordPress/twentytwentytwo/issues/234

kjellr commented 2 years ago

In general, this is looking promising. I know this is in progress, but leaving a few notes here:

Many of our full-width patterns provide their own padding to ensure their inner content doesn't bump up against the edges. But currently, they are bumping up against the edges even so:

Screen Shot 2021-12-10 at 9 50 34 AM

Also, I think we need to be more specific about these rules. Here's an example of the post content placed inside of a column. As you can see, the left/right negative margin is being applied when it shouldn't be:

Screen Shot 2021-12-10 at 10 01 24 AM
MaggieCabrera commented 2 years ago

Thanks @kjellr! Generally, I've found in most cases the only problem is that we are applying full width where we really don't need to do so. Such is the case of the first pattern that you mention (I've just fixed that). I'll have a look at the other case, but I think we could use as many eyes on this as possible so we cover all the possible cases.

I'll clean up this PR so that it's ready for review.

kjellr commented 2 years ago

we are applying full width where we really don't need to do so

Could be. But we also need to ensure there are fallbacks in place because we can't control what weird stuff other people do. This is exactly why the layout rules tend to get so complicated. 😅

MaggieCabrera commented 2 years ago

@kjellr how did you manage to get that full width item inside the two column layout? it doesn't let me:

Screenshot 2021-12-10 at 16 51 36
kjellr commented 2 years ago

I set the Query block there to inherit the default layout. Generally that's a bad idea in this setup, but I was trying to break it and find holes. 😄 When I did this, it looked fine in the editor, but broke in the front end.

kjellr commented 2 years ago

I pushed a few small fixes to templates. In general, this is looking pretty good — I've been running through a bunch of content and haven't found any major errors. We should try some tests where there are alignfull items inside of columns though, cause I expect there could be issues.

I'm going to mark it as ready for a true review.

MaggieCabrera commented 2 years ago

I introduced the exception for columns, I think this is the intended behavior, right?

Screenshot 2021-12-13 at 14 09 27

This is the markup I sued on the example above:


<!-- wp:columns {"align":"full"} -->
<div class="wp-block-columns alignfull"><!-- wp:column {"backgroundColor":"primary","textColor":"background"} -->
<div class="wp-block-column has-background-color has-primary-background-color has-text-color has-background"><!-- wp:paragraph -->
<p>1 column full aligned</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column {"backgroundColor":"secondary"} -->
<div class="wp-block-column has-secondary-background-color has-background"><!-- wp:paragraph -->
<p>column 1</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column {"backgroundColor":"secondary"} -->
<div class="wp-block-column has-secondary-background-color has-background"><!-- wp:paragraph -->
<p>Column 2</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"inherit":true}} -->
<div class="wp-block-group"><!-- wp:group {"align":"full","backgroundColor":"primary","textColor":"background"} -->
<div class="wp-block-group alignfull has-background-color has-primary-background-color has-text-color has-background"><!-- wp:paragraph -->
<p>group full aligned</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:paragraph -->
<p>---</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->

<!-- wp:columns {"align":"full"} -->
<div class="wp-block-columns alignfull"><!-- wp:column {"backgroundColor":"secondary"} -->
<div class="wp-block-column has-secondary-background-color has-background"><!-- wp:paragraph -->
<p>column 1</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column {"backgroundColor":"secondary"} -->
<div class="wp-block-column has-secondary-background-color has-background"><!-- wp:paragraph -->
<p>Column 2</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"inherit":true}} -->
<div class="wp-block-group"><!-- wp:group {"align":"full","backgroundColor":"primary","textColor":"background"} -->
<div class="wp-block-group alignfull has-background-color has-primary-background-color has-text-color has-background"><!-- wp:paragraph -->
<p>group full aligned</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:paragraph -->
<p>---</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
kjellr commented 2 years ago

That looks about right to me.

A couple general questions I thought of this morning:

MaggieCabrera commented 2 years ago

Ugh, I just noticed the 1 column example above is not correct, I'll have another pass at that.

MaggieCabrera commented 2 years ago 
* Is there anything to this approach/CSS that could be ported upstream to Gutenberg instead? At a glance, this code isn't very specific to this theme. If we merge this, I expect other themes will copy/paste this code, and if possible it might be better to just place it in Gutenberg somewhere so it can be revised and replaced in the future more easily.

I don't think so, no. It needs more testing, to see if we've missed something somewhere we don't want the negative margins besides the columns case. The idea for GB was to be able to handle the padding variable with a built-in variable so themes could opt-in into this, else you will break themes that implement this differently or don't want this behavior at all.

* We should test alongside the current implementation of sit padding, and make sure it doesn't break horribly.

I don't know what you mean exactly, what implementation?

MaggieCabrera commented 2 years ago

@kjellr for the case of a full width block inside a one column that is not full width, the current behavior doesn't make the inner block go all the way to the borders because of the fixed width of the parent so I don't think we need a rule for those. I'm removing that exception

MaggieCabrera commented 2 years ago

The group thing, on trunk:

Screenshot 2021-12-13 at 14 30 54

On this PR:

Screenshot 2021-12-13 at 14 30 37
MaggieCabrera commented 2 years ago

This is ready for another look, I don't have any broken cases in my arsenal, please try and break this

kjellr commented 2 years ago

Are you seeing extra padding in the site editor?

Front End Site Editor
Screen Shot 2021-12-13 at 9 33 20 AM Screen Shot 2021-12-13 at 9 33 30 AM
MaggieCabrera commented 2 years ago

Are you seeing extra padding in the site editor?

What I'm seeing is that the alignfull on that block is only working because the block has the class (in the front), but that block doesn't have alignment options showing up on the toolbar, so the editor doesn't add the data attribute that we are targeting in the editor. So I think that's a case of refactoring the template part, do you see it somewhere else?

kjellr commented 2 years ago

Ok I see, we have to add an additional group wrapper there. 😅

Pushed https://github.com/WordPress/twentytwentytwo/pull/291/commits/6210863452753091f83d3252421e8e5c7c765f92 to take care of that.

jffng commented 2 years ago

I made three changes to try and resolve the issues reported above:

kjellr commented 2 years ago

I found a use case that doesn't work right: A full-width Group block with a background color. The full-width child here is supposed to go to the edge, but it doesn't. I'll try and push a fix.

Screen Shot 2021-12-14 at 9 12 04 AM
kjellr commented 2 years ago

I fixed that, and also synced things up so that wide/full blocks inside of a full-width group block are aligned with wide/full blocks outside of one:

Before After
Screen Shot 2021-12-14 at 9 33 10 AM Screen Shot 2021-12-14 at 9 32 52 AM

At the moment, the only issue I'm seeing is that full-width items inside of non-full width parents incorrectly inherit the negative margins and padding:

Screen Shot 2021-12-14 at 9 49 41 AM

As far as I can tell, we can't really clear that without breaking all kinds of other stuff. So I think we just have to let it be.

jasmussen commented 2 years ago

TT2 is my favorite theme in years, in part because it's able to do so much with just blocks alone. It's clearly shown the need to solve this at the block editor level. It's also shown how tricky it is to get that right, and so I think it's good to buy some time with this solution. Hopefully the usage and rules can help us zero in on the right heuristic to implement.

As for the specific rules, it's definitely a little hard to parse if you haven't built a ton of the CSS. At a high level, the code looks good for me, and in testing, things look right to me:

Screenshot 2021-12-14 at 16 34 26 Screenshot 2021-12-14 at 16 34 32

I also tried some patterns out, and it all appears to work well. The GIF was too big, so it's here.

I would love for the clamp rules to be absorbed by the system as well — the inability to see or intuitively edit the some of the paddings feels a little janky. But that's a separate issue, and probably not one that needs solving in the near future.

Ultimately, if we go with this approach, which as noted I think is good to do for the time being, it seems okay to merge soon and then keep an eye out for anything that might need following up on.

kjellr commented 2 years ago

Thanks for the testing! I also tested with the current Site Padding implementation, and it works fine... it just adds padding to the outside of all this:

Example Example
Screen Shot 2021-12-14 at 11 01 48 AM Screen Shot 2021-12-14 at 11 02 16 AM

I'd like to reiterate what I said over in this other thread:

This is not our ideal solution, but it's the least complicated one for today. It's a handful of CSS rules, and it generally works without the user changing their behavior or opting into anything. The end goal is to get a solution merged into Gutenberg, but I think this is a first step towards that and is worth testing during the upcoming Beta cycle.

MaggieCabrera commented 2 years ago

Thank you all for testing this, it's a complicated problem!

richtabor commented 2 years ago

Hey all 👋 — love this. I noticed that there's a discrepancy between the use of the spacing.outer var and the padding set by various patterns. If you modify the outer value in theme.json, you'll see that the patterns don't adjust as well (as many set their own padding left/right values.

I took a stab at it in #310 — which lets the patterns adapt to the outer value. Let me know what you think :)