Full page patterns have redundant nested groups which makes section styles unpredictable

[jasmussen]

Description

The default full page patterns have multiple nested redundant groups. This conflicts with zoom out mode and section styles, which is best described first visually in this GIF:

Step-by-step reproduction instructions

1. Go to site editor.
2. Create a NEW page, give it any title.
3. Choose the "Business homepage" pattern.
4. Click a section to change the section style to a different color.
5. Now zoom out, observe how a different section style is chosen, and choosing a new section style isn't actually applied.

What happens here is that zoom out mode works with top level sections, allowing you to select only those. Which means if there's a wrapper group that is not the source of the section style, then there will be a perceived invisible disconnect, as the section style you just applied is one level deeper, on a block no longer selected.

Expected behavior

If I set a section style not zoomed out, then zoom out, I should see the same section style. Ideally, we can remove the redundant wrapper groups. Observe in this screenshot, for example, how the "fleurs co" pattern has two wrapping groups:

Because of the two groups, when you click not zoomed in, you'll select the most nested one. But if you click when zoomed out, you'll select the least nested one.

Environment info

6.7-rc2

[jasmussen]

@juanfra @carolinan @beafialho do you know why there are additional wrapping groups in these patterns? And would they be safe to remove?

[juanfra]

@jasmussen the wrapping groups were added here, and it was in combination with some other fixes, trying to resolve everything that was reported here.

These wrapping groups were added because the landing pages were built (originally) using one group that wrapped all inner patterns, and that was causing interferences with the zoom-out view. The other reason is that we needed to set a zero margin between the groups so that the layout respected the design (no vertical gap between full-width groups). If we put one pattern after the other, we would have a gap between them.

However, we can try to see how removing the groups from these pages reacts to the most recent improvements to the zoom-out functionality.

[jasmussen]

Oh thanks so much for updating me on those details. @richtabor and @MaggieCabrera I'd love your take here, because the added wrapping groups may have solved that issue at hand, but as noted in this issue, it caused others. At the same time, I just recorded a zoom out featurette here, where I removed these wrapping groups specifically so I could properly demo zoom out mode, and everything worked fine for me!

As for the margins, I simply applied zero margins to the inner groups, rather than have that margin rule sit on the outer groups. Works the same for me!

[carolinan]

Adding margin 0 to the inner groups only works when the pattern is followed by another (full width) pattern. In other scenarios, it removes the spacing that separates the pattern from other blocks, including paragraphs.

[richtabor]

This is a big issue. Makes the theme feel broken.

[richtabor]

Why not just remove the duplicate wrapping group blocks and do something like this instead?

.alignfull + .alignfull {
	margin-top: 0;
}

[carolinan]

Because it would affect all full width blocks not just the selected patterns.

[carolinan]

What if we duplicate the pattern code on these landing pages and add the margin reset, instead of using the individual patterns?

[jasmussen]

What if we duplicate the pattern code on these landing pages and add the margin reset, instead of using the individual patterns?

Not entirely sure I'm following, so forgive me if I'm missing nuance. Do you mean instead of doing the pattern include, to deduplicate theme code, we just copy verbatim and have the code live in two places?

If this would let us avoid the redundant wrapping groups, and still let me use section styles on the individual top groups, in zoom and unzoomed both, then I'd be for it, absent any other perhaps more elegant solutions. It's still unclear to me why it wouldn't be possible to simply set zero margins, top or bottom or both, on these page patterns. I'm sure there's also context I'm missing here.

[juanfra]

I'm not convinced about duplicating the patterns. It'll likely create confusion and double maintenance.

It's still unclear to me why it wouldn't be possible to simply set zero margins, top or bottom or both, on these page patterns.

@jasmussen, we initially had zero margins for all the full-width patterns with background and then changed that here. The reason was that having no margins had the side effect of the block being too close to other types of content.

[jasmussen]

@jasmussen, we initially had zero margins for all the full-width patterns with background and then changed that #328. The reason was that having no margins had the side effect of the block being too close to other types of content.

Another option, if you wanted to combine the full-width patterns with regular content-column text like in the example, could just be to add a linebreak, or a spacer. But the motivation is valid enough.

What I mean in this case, though, is to locally add the zero margin in the full page pattern. This appears to be what's already being done, just on the redundant groups. So they seem to exist primarily to avoid duplicating the patterns, yes?

In that case, and outside of revisiting the decision to remove the zero margin, we are left with a choice of whether to have a better developer experience for the TT5 theme at the cost of the user experience, or a better user experience at the cost of the TT5 dev ex. Is that a valid articulation?

[juanfra]

Another option, if you wanted to combine the full-width patterns with regular content-column text like in the example, could just be to add a linebreak, or a spacer. But the motivation is valid enough.

I agree.

This appears to be what's already being done, just on the redundant groups. So they seem to exist primarily to avoid duplicating the patterns, yes?

Exactly. The multiple wrapping groups are there because:

• If we used one group containing all the patterns with block gap zero, zoom out detects that as a full section, and it doesn't let you insert patterns in between.
• As we couldn't go with the option above, we used multiple wrapping groups to remove the margin between patterns, to match the design. This way, we could also allow the insertion of patterns in between in zoom-out.

So, the reasons were primarily matching the design, and allowing the insertion between patterns in zoom-out. The question of duplicating the patterns is new and a result of the latest findings.

In that case, and outside of revisiting the decision to remove the zero margin, we are left with a choice of whether to have a better developer experience for the TT5 theme at the cost of the user experience, or a better user experience at the cost of the TT5 dev ex. Is that a valid articulation?

Yes, we can question that. It's a tough one because there are good arguments for both options.

We could also ask ourselves why in zoom-out we only have controls for Styles sections, and not any other control? Maybe a different angle could be removing the section styles for zoom-out view. It doesn't feel too cohesive to have only one control there. This could also solve the problem.

[jasmussen]

We could also ask ourselves why in zoom-out we only have controls for Styles sections, and not any other control?

Valid enough question, but the answer is because zoom out exists to provide a literally receded view where you work at a high level, only with top level sections and styles that get inherited. It's a means to flatten the hierarchy with a goal of simpler high level orchestration of the content. In this case, the styles are definitely meant for section level items (even if you can use them for any group), so I find it a good pairing. And in this case, since the redundant groups exist mainly for maintainability of the theme, absent better ideas, I'd fall towards the angle of the user experience here.

[richtabor]

We could also ask ourselves why in zoom-out we only have controls for Styles sections, and not any other control? Maybe a different angle could be removing the section styles for zoom-out view. It doesn't feel too cohesive to have only one control there.

There will likely be more high-level controls added over time.

[richtabor]

So they seem to exist primarily to avoid duplicating the patterns, yes?

In that case, and outside of revisiting the decision to remove the zero margin, we are left with a choice of whether to have a better developer experience for the TT5 theme at the cost of the user experience, or a better user experience at the cost of the TT5 dev ex. Is that a valid articulation?

I'm 100% for prioritizing user experience. It doesn't matter how clean, how organized, how thoughtful technical solutions are - if the user can't succeed with those decisions.

Knowing what we know now, we can either:

1. Remove wrapping group blocks on the page patterns and zeroing out the top/bottom margin on relative patterns—even though patterns will be inserted right up against text.
2. Remove the wrapping group blocks on page patterns and copy the contents of those patterns into the page pattern file, zeroing out the margin there—even though there's much more overhead maintaining patterns, now that page patterns are more unique.

I lean towards option 1.

[juanfra]

Given the different trade-offs of the various options, it sounds reasonable to go with option 1. Having a few patterns with zero margin should only imply the spacing problem among consequent elements, which the user can solve by breaking the line, spacer, or spacing in the other elements.

I'll open a PR with the proposed approach, modifying only the margin of the patterns included in the landing pages.

[richtabor]

I'll open a PR with the proposed approach, modifying only the margin of the patterns included in the landing pages.

Sounds good.

Just confirming, #667 is not the current direction, yea?

[juanfra]

Just confirming, #667 is not the current direction, yea?

@richtabor yes, I've just created the PR with the approach we agreed here