Alternative approach to the layout outer padding #36214
Conversation
youknowriad
added
the
[Feature] Themes
youknowriad
requested review from
ajitbohra,
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
@youknowriad: @jffng and I are both getting editor errors when we try this PR:
Error: |
|
yep, should be fixed now. The padding defined in the layout should be something like: |
youknowriad
force-pushed
the
update/alternative-approach-outer-padding
branch
from
abc42c4
to
bb569ee
Compare
|
Size Change: +222 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
|
Alright thanks — I can access the editor again. I drafted up a quick PR in WordPress/twentytwentytwo#209 to test this against.
EDIT: Nevermind, the suggestion in WordPress/twentytwentytwo#209 (comment) got this working! |
|
This works pretty well! The only major hesitation I have is this part:
Here's a visual representation of this:
The theme has left/right padding and no top padding, so that's what's applied to the block when inherit layout is true. There's no way to modify that padding at all. I feel like we should be able to still access padding controls, and if we edit those, the negative margins should just shut off automatically. Is that possible? |
I'l reluctant because this is not how it works with the layout styles (widths). When you inherit, you ignore values defined in the block itself. So if we change it for padding, I wonder if we should be consistent. So: if block specific value is defined use it, otherwise use the inherited value. Also, the question here is do you think we should prefill the padding control with the layout inherited value? Also, how do you clear the value? Right now clearing a value means making it undefined which would revert to the inherited value, it could become confusing. |
|
Yeah, that makes sense. This is a complicated problem. 😅 I'm thinking that we're probably not ready to merge this for the feature freeze — it doesn't feel quite ready. Since it's really just an iteration on existing functionality we can probably keep thinking through it through Beta anyway? I'm not sure. |
|
I agree that there's no need to rush it 👍 . |
Whew, this is a complicated one. So you're thinking that if the padding control is modified, then the negative margins wouldn't work any longer? Other question, this doesn't solve the fullwidth/negative margins for any other alignfull blocks, correct? |
That's right. My general thinking is that if you've added padding to a specific block this way, you're overriding the defaults and choosing to have that padding surround every interior block. That generally works for my mental model, but I'm not sure how universal it is. Like you said, this is a complicated issue!
The PR currently works for any alignfull block whose direct parent inherits the default layout. |
|
Here's an example of where I found the automatically-added padding confusing: In the post/page editor, I have a Group block that's wide-aligned. I've activated "Inherit default layout" because I'd like this block to contain one wide-width block, and one standard-width text block. Here's how that looks today:
This is as I'd expect it to be. The Group block parent takes up the full "wide" width of the page, and so does the wide-width child. The standard-aligned block is smaller than that. The site padding exists, but it's on the left and right of the entire block (as expected). But with this PR active, the Group block parent gets extra padding, so it displays like this:
There's no need to double-up on the padding here. The "site padding" is already present outside of this block. And there's no way for me to remove it currently. |
For me the issue here is that you should have never used "inherit layout" on that group block, instead you should have defined a custom layout (custom widths), the reason is simple: the page canvas is full width, meaning a layout that is suitable there (the default layout) is not something you should apply to a "wide" block because it breaks there (regardless of padding). Example: the theme defines: In other words for me: |
What's the benefit to us having this control available on blocks that are not full-width then? Can/should we filter that out? The majority of blocks the user sees these controls on do not fit that criteria. |
|
@kjellr Think of it as a preset for theme authors, so they don't have to redefine the same layout over and over across blocks and templates. I'd love to find a way to hide it in the situations where it makes less sense but it's very hard to know statically. For instance it makes sense in a query loop inside a group inside a group inside a group inside a template part if the top level is full width. |
youknowriad
force-pushed
the
update/alternative-approach-outer-padding
branch
from
8f1048b
to
d241ae2
Compare
|
Where are we at with this one? I think it's probably close, but I still think having the option to clear these paddings for child blocks is important. If that's something we'd consider trying to add, do we need Design to help figure out answers for these questions?
|
|
I'm personally not confident about showing the padding control or allowing overriding the "default inherited one" because it creates a precedent without how the inherited layout properties work.
I don't think it's something to be done specifically for this PR but more something to be thought of holistically and not something we can do properly for 5.9. |
|
I'm personally happy to ship this as is if folks thinks it's a decent compromise. I can't think of a better one for now that won't have bigger impacts. |
|
I'm still pretty uncomfortable with because it takes an already complex control (Layout) and adds additional complicated behavior to it. Here are a few of my concerns:
We could adjust the label to say "Inherit default layout and padding", but the layout control is already pretty opaque to the user (cc #36675), and I'm not sure if that helps or adds complexity.
This content in the middle of this block is wide-aligned, and sits inside of a full-width gray Group block. The padding on all four sides is defined on the parent Group block. It's generally pretty easy to create. With this PR though, this pattern looks like this by default:
I can't override that
Most users won't figure out how to make that happen — it's far less intuitive than just editing the padding for the Gray Group block. |
|
It feels worth adding that for TT2 we did discuss whether we couldn't handle the challenge in CSS. It has its own tradeoff, but it could buy some time to figure out a better solution, potentially. |
|
The CSS solutions that I saw so far will solve some use-cases but create issues for others, I don't believe there's a perfect CSS solution for this. What makes this hard is that ideally, there shouldn't be any user or theme author interactions, the premise is simple: Applying a full width alignment to a block should ignore the padding of its container, the reality is that we can't write a CSS rule to achieve that in a generic way. |
That's a good summary! This is truly difficult. 😅 At the moment, I'm primarily trying to consider the best-case scenario for 5.9... Since we don't have a solution we all feel solid about, I think anything we land at this stage needs to be relatively minimal and mostly harmless: Something automatic, that can be stripped out and replaced if need be. It would be good to get something into today's Beta, since it's the last (confirmed) beta before the holidays. I lean towards the fix in WordPress/twentytwentytwo#291 because it mostly works, and is just a handful of CSS rules. I think the main thing going for it is is that it's replaceable without setting precedent, and it buys us a bit more time. For now, I think we should just try putting that in and seeing how it tests during the next beta cycle. |
I can be onboard with that but let's keep it to the real CSS minimum override and also add a clarifying comment that block themes shouldn't rely on it too much as it might break or become unnecessary in the future. Themes will consider 2022 as the theme to copy for everything. |
Fully agreed. 👍 |
That's why it's important that the fix is in the editor, so future themes don't include these extra rules that will potentially be obsolete in the near future in favor of a better solution. I think the TT2 styles on WordPress/twentytwentytwo#291 with the variable for controlling the padding amount via theme.json should be a good starting point to make it opt-in. |
|
FYI - the number of changes in layout, etc. made a rebase a bit messy, so have cherry-picked the changes onto a fresh branch off trunk over here so we can do some more exploration with this. |
|
Interesting @glendaviesnz I'm closing this now |
Alternative to #35919
Instead of using a dedicated
outerPaddingproperty on the layout object. This PR usespadding, so for instance if a user sets a padding to a group block and also add the same time defines content/wide widths, the padding applied to the block is used to compute the negative margins.That said, when the layout is defined as "inherit: true", the padding control in the container block is hidden and the "padding" defined in the theme.json file (settings.layout.padding) is used instead.
What do you think? It feels a bit better than #35919 because there's no padding conflicts in the UI.
Cons:
!important