Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove color, spacing, and layout options for Template Part block #36571

Merged
merged 3 commits into from Nov 29, 2021

Conversation

jameskoster
Copy link
Contributor

@jameskoster jameskoster commented Nov 17, 2021

As discussed in #30732, allowing folks to apply style to the template part block, in addition to the styles within the template part can lead to confusion.

For example if you add a background color to your header template part whilst editing the Index template, that change will only be applied to the Index template. This is due to the block styles being local to the parent template rather than the template part. The confusion arises because this is conceptually antithetical with the idea of Template Parts existing as discrete global entities. When users edit their header, they expect those changes to be applied globally. To add even more confusion, when you then go to edit the template part in isolation your changes aren't present. This is highlighted in the following video:

140041721-870e4e15-317e-42b1-b2eb-f4760fa7c98e.mp4

This PR removes those options from the block.

The main drawback here is that it removes the option of providing per-template-overrides for template parts, which can occasionally be a useful feature for theme authors. However this "feature" is ultimately the cause of the aforementioned confusion so an alternative is required. Fortunately it is very easy for theme authors to work around this by creating additional template parts as required, which is a practise more conceptually aligned with the Template Part featured anyway.

Unfortunately I can't think of a way to make this change backwards compatible, so would love to discuss options there.

@jameskoster jameskoster added [Block] Template Part Affects the Template Parts Block [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Nov 17, 2021
@jameskoster jameskoster added this to 👀 Needs review in WordPress 5.9 Must-Haves via automation Nov 17, 2021
@kjellr
Copy link
Contributor

kjellr commented Nov 17, 2021

Just a heads up that we currently apply padding to just about every template part block in Twenty Twenty-Two.
For example:

https://github.com/WordPress/twentytwentytwo/blob/76dc10d8fad1c3354bfd3dad19016547a39e0954/block-templates/index.html#L1

This is is primarily a stopgap until a better solution to site padding is in place. If this PR were to land before that, we'll need to probably add some additional Group wrapper blocks. We might need to do that anyway though — we apply layout to most of these blocks too, and we definitely want to preserve that behavior.

Either way, just want to flag that this would require a bunch of Twenty Twenty-Two rework. (cc @jffng so you're aware too)

@jameskoster
Copy link
Contributor Author

Yup, that's the drawback here. Any themes making use of this affordance would need to be slightly refactored. But like you said, it really should just be a case of adding a Group, and applying the styling to that instead.

@jameskoster jameskoster linked an issue Nov 17, 2021 that may be closed by this pull request
@jameskoster
Copy link
Contributor Author

jameskoster commented Nov 17, 2021

One idea to provide backwards compatibility here would be to conditionally show/hide the UI rather than fully disabling the features.

So if a theme has set padding on the Header in the Page template, the padding control would be visible while editing that template. Otherwise the padding control is hidden from the UI and cannot be added.

Edit: A similar but more aggressive approach would be to make the feature code-only. So you can add padding to template parts in the template.html file, but users will not be able to change those values in the UI.

@shaunandrews
Copy link
Contributor

This feels like the wrong solution to the problem.

This is due to the block styles being local to the parent template rather than the template part.

This feels like the part that needs to be fixed. In many ways, I think of template parts like div's, and as such, I should be able to apply options (like color, spacing, and layout) to a template part. Having to add an unnecessary Group block to resolve this feels like it defeats the purpose of the template part block.

I could actually see an argument that the current behavior is actually desirable, allowing one template part to behave differently depending on the template that contains it.

@jameskoster
Copy link
Contributor Author

This feels like the part that needs to be fixed

The thing is, block styles being local is fundamental principle of the block editor. We did explore making them "sync" with the template part so that they are applied globally in #30732 (comment).

But this means you'd lose the per-template override option, and surfaces a technical problem – it's not currently possible to apply styles to a document (the template part in this case). So to do this we'd need to break an existing heuristic and open a can of worms on the tech side.

I could actually see an argument that the current behavior is actually desirable, allowing one template part to behave differently depending on the template that contains it.

I agree it can be desirable, but not in the majority of cases. I don't think this should be the default behaviour. As outlined in #30732 the expectation is that any changes made to the template part are applied globally, which isn't what currently happens.

We need to entertain both options – changes being local, or pushed upstream to the template part itself. The latter feels like it should be the default experience, and the former is a complex feature that needs more design consideration in order to make it feel intuitive.

All this considered, the additional group block feels like the lesser of two evils. It solves the original issue without breaking an existing convention or adding any technical overhead, and gives us time to work out the nuance of template part overrides.

@carolinan
Copy link
Contributor

carolinan commented Nov 19, 2021

Just a heads up that we currently apply padding to just about every template part block in Twenty Twenty-Two.

Sounds like it might be good to add it as a default in theme.json instead?

@kjellr
Copy link
Contributor

kjellr commented Nov 19, 2021

Sounds like it might be good to add it as a default in theme.json instead?

Maybe, but that seems like a tangent, and I hope it goes away entirely anyway. 😅

@overclokk
Copy link

I talk about this issue 9 months ago (nine) #29228 but no one listened to me and now problems are popping up like mushrooms, I'll repeat here, wp:template-part must work like get_template_part() only includes the template you want to include and stop, no other behavior or it'll add side effect like this one, so please, remove also the div wrapper that is useless.

If you want to have a different style for a template part in a specific page, create a new template part.

@ellenbauer
Copy link

I could actually see an argument that the current behavior is actually desirable, allowing one template part to behave differently depending on the template that contains it.

That would be a smart approach in my opinion, as it would be cool to have the possibility to just change the background of a header template part without the need to create a second template part for the second color. This can add up to a lot of header template parts on a colorful website :)

@ellenbauer
Copy link

Can I add one question here: Would it still be possible to add a class name to wp:template-part even if color, spacing and layout options are not supported any longer?

@overclokk
Copy link

@ellenbauer If you need different style for a template part why not just using pattern instead of template part, wp:template-part should not add any style.

@ellenbauer
Copy link

@overclokk Thanks a lot for your answer, by pattern, do you mean the new pattern block?

@jameskoster
Copy link
Contributor Author

Would it still be possible to add a class name to wp:template-part even if color, spacing and layout options are not supported any longer?

Yes. The parent template is also added as a class to the body tag.

If you need different style for a template part why not just using pattern instead of template part, wp:template-part should not add any style.

Another option would be to insert the template part inside a group, and add a background color to that group:

Screenshot 2021-11-22 at 10 20 00

@Andrew-Starr
Copy link

Yes please remove all styling options from wp:template-part

Like others have said, it will only get exponentially more confusing as more block themes are released, and more and more non-technical users begin activating them.

We need to get ahead of this now before it becomes too late to even consider reversing it.

Imagine a new WordPress user who changes the background color of the header template part on the index template, and their frustration when this change is not visible when viewing pages, posts etc.

Inevitably they will naturally assume there is a problem with the theme not working correctly.
How do we then explain to the user why this "issue" is happening, in a non-technical way that they can understand?

And then imagine having to provide ongoing help and support for this problem forever...

@overclokk
Copy link

@ellenbauer yes, the block pattern. Very usefull also for template variations.

@carolinan
Copy link
Contributor

As it is still a container block, would it not need to keep the layout option? Or support it but not show the control.

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a change for the better and will reduce confusion. 👍

Code-wise it's a minimal change and the code looks good.

@overclokk
Copy link

overclokk commented Nov 24, 2021

The div wrapper remains to be removed as well.

The "tagName":"header" I means.

@MaggieCabrera
Copy link
Contributor

+1 on removing the div wrapper here

@aristath
Copy link
Member

I'm not sure that removing the tagName (and therefore the wrapper) would be viable...
A template-part should be a <header>, <main>, <footer> or <aside> and without the tagName we won't be able to mark them as such. The <div> is a fallback and not the norm.

@jameskoster jameskoster merged commit 877e5ce into trunk Nov 29, 2021
WordPress 5.9 Must-Haves automation moved this from 👀 Needs review to ✅ Done Nov 29, 2021
@jameskoster jameskoster deleted the update/template-part branch November 29, 2021 13:27
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 29, 2021
@mtias
Copy link
Member

mtias commented Dec 1, 2021

Some context that wasn't incorporated in the discussion here is that template parts are also going to become viable style providers. What that means is that a wp:template-part node won't just be referencing inner blocks stored elsewhere, but also wp_style objects (think subsets of theme.json) if one exists for that same reference/slug. These template part style objects would allow configuring things like color palettes, default font sizes, etc, at the scope of a template part alone. This is definitely more of an advanced use case, but it's worth keeping in mind as it's not directly comparable to using a group within.

@jameskoster
Copy link
Contributor Author

Seems like that would be a good point to explore in more detail the idea of local style overrides for template parts.

@carolinan carolinan added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jan 5, 2022
@carolinan
Copy link
Contributor

In 5.9-RC1-52446, the color settings are not removed from the template part. Adding the backport to minor release label.

@dianeco
Copy link

dianeco commented Jan 11, 2022

Does it mean that these options will be available for WP 5.9 and later removed in a minor release?
If that's the case, IMO, it will be confusing for users who have previously built using these options to not be able to modify them anymore (or worse, if it changes the design of what they've created).

@jameskoster
Copy link
Contributor Author

Agreed, this could cause a few headaches if it's not included in 5.9. Is it definitely too late? 🤔

@kjellr
Copy link
Contributor

kjellr commented Jan 11, 2022

cc @noisysocks for thoughts about inclusion at this point.

I do expect a major uptick in block theme creation as soon as 5.9 is released, and delaying this would potentially cause breakage later on. There's no really fallback in place, right? Once these features are removed, they're gone.

@noisysocks noisysocks added Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Jan 12, 2022
@noisysocks
Copy link
Member

It should be okay to include this in RC 3 as I don't see any string changes or new code in the diff.

@carolinan
Copy link
Contributor

That's a big relief. It did not get any traction when I brought it up on Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Template Part Affects the Template Parts Block [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing")
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Style changes made to template parts are only applied to one page