WordPress / twentytwentytwo

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

Remove inline styles from templates #318

Closed fklein-lu closed 2 years ago

fklein-lu commented 2 years ago

Describe the bug

A block theme can by definition not use any feature that isn't available to end users. Hardcoded CSS is such a feature, and therefore a no go.

To Reproduce

Steps to reproduce the behavior:

  1. Go to the Site Editor
  2. Open the Single template.
  3. Inspect the Group block that contains the Post (the one immediately below the header).
  4. See padding being applied to the Group block.
  5. Check block settings, check theme.json. No trace of where this originates from.
  6. Check the template HTML, see that this is hardcoded.

Expected behavior

All paddings or margins should either be added through the Site Editor interface. If the available UI is not a good fit, then a block style should be used.

carolinan commented 2 years ago

Hi I'm trying to locate the padding you are talking about in single.html. Which line number? "the group immediately below the header": I do not see the padding? https://github.com/WordPress/twentytwentytwo/blob/trunk/templates/single.html#L3

fklein-lu commented 2 years ago

I double checked, and these have since been removed. But these changes weren't synced back to Core.

There's still inline CSS in the header (https://github.com/WordPress/twentytwentytwo/blob/trunk/parts/header.html#L3) and footer (https://github.com/WordPress/twentytwentytwo/blob/trunk/parts/footer.html#L2).

kjellr commented 2 years ago

I double checked, and these have since been removed. But these changes weren't synced back to Core.

The theme was synced up to core yesterday, in preparation for the RC1 release. I can't spot a mismatch in single.html, but if you're still seeing one, please let me know where.

There's still inline CSS in the header (https://github.com/WordPress/twentytwentytwo/blob/trunk/parts/header.html#L3) and footer (https://github.com/WordPress/twentytwentytwo/blob/trunk/parts/footer.html#L2).

The inline styles there are part of the standard Gutenberg markup for a Group block, and are synced up with the commented markup above. If you add a group block and set padding for it, you'll see that inline markup is added:

group

fklein-lu commented 2 years ago

Cheers, but this is what I see in the header template:

<!-- wp:group {"align":"wide","style":{"spacing":{"padding":{"bottom":"var(--wp--custom--spacing--large, 8rem)","top":"var(--wp--custom--spacing--small, 1.25rem)"}}},"layout":{"type":"flex","justifyContent":"space-between"}} -->
<div class="wp-block-group alignwide" style="padding-top:var(--wp--custom--spacing--small, 1.25rem);padding-bottom:var(--wp--custom--spacing--large, 8rem)">

So how do you get the var(--wp--custom--spacing--large, 8rem) in there through the interface? Because when inspecting the block, nothing shows up. Which is different to this being added through the interface as shown in the animation above.

kjellr commented 2 years ago

Gutenberg supports custom values for spacing in most contexts (var(), min(), clamp(), etc.). These are all valid use cases, but are mostly for theme authors and power users to add. There's work underway to treat these values more appropriately in the UI: https://github.com/WordPress/gutenberg/issues/35558

fklein-lu commented 2 years ago

These are all valid use cases, but are mostly for theme authors and power users to add.

You stated the issue that I see with the approach of hard coding styles very eloquently. Twenty Twenty Two is supposed to showcase the power of the full-site editing as it is right now. Not what it can do in the future, relying on workarounds in the meantime.

I find it crucial that users can discover how the theme is built through the Site Editor, and only through the Site Editor. I'm very familiar with full-site editing, but even I was perplexed about these seemingly magic paddings at first.

Furthermore if a user decides to change a template, and in the process removes the Group with the hard coded styles, there's no recreating it again. This clashes with the principle that all user actions in the Site Editor should be revertible.

A look at the code shows that in the templates themselves, only very few uses of hard coded styles remain. So why not remove these as well?

I agree that the visual outcome is better if you rely on the flexibility provided by CSS. But at the same time the theme also uses spacer blocks, which don't adapt at all to screen size changes, as their height is fixed in pixels.

So using a single value for the padding in the header and footer will make the theme's layout on mobile a bit more awkward. But not more awkward that it already is, due to the existing limitations of the spacing controls at this time.

carolinan commented 2 years ago

(The templates can be reset, but it removes all changes not only the group)

fklein-lu commented 2 years ago

Right, but it's not only that. What if I want another Group with this padding?

kjellr commented 2 years ago

Thanks for the feedback on this. I do appreciate the arguments for not using variables here — I've gone back and forth weighing those same concerns myself, so I totally understand the reasoning.

In general, I personally found it better to use these variables than to stick to static values. There were just far too many areas of the design that would benefit from having dynamic values: everything from the impactful large text in the theme screenshot to the spacing around various elements. When compounded together, using static values for all of these led to either a significantly toned down version of the theme (where everything was way smaller on large screens), or a broken one (where everything is too large on small screens).

I also don't truly consider these variables a workaround — Gutenberg supports them today by design. I do hope that Gutenberg will eventually make some of this variable usage obsolete, but in the meantime this approach helps provide solid responsive behavior from day one. And I think that's table stakes for most websites these days. There are definite tradeoffs, but there's also room for Gutenberg to help smooth those over in the future.

In any case, I do appreciate the discussion, but these variables are used extensively through all of the theme's templates and patterns, so at this point in RC this is not something we can re-evaluate.

kjellr commented 2 years ago

Now that we're nearing release, I'm going to close this issue. If anyone has further thoughts to share on this topic, feel free to open up a new issue on Trac.