WordPress / theme-experiments

Experimenting with themes made out of blocks.
GNU General Public License v2.0
546 stars 179 forks source link

Update TT1 Blocks to use Gutenberg alignments #236

Closed kjellr closed 3 years ago

kjellr commented 3 years ago

See #234. This PR is just like #233, but for TT1 Blocks.

This looks great in the editor, but I'm not seeing the new widths reflected in the front end at all. Is there something I'm missing? 🤔

Editor

Before After
gutenberg test_wp-admin_post php_post=2 action=edit gutenberg test_wp-admin_post php_post=2 action=edit (1)

Front-end

Before After
gutenberg test__page_id=2 (1) gutenberg test__page_id=2
kjellr commented 3 years ago

I think this PR is up to date, but it doesn't work as expected:


1. When I set the template parts to "Inherit default layout", all of their inner blocks appear at a default width.

Screen Shot 2021-03-22 at 11 38 15 AM (^ The containing columns block there is supposed to be wide-width.)


2. I'm having trouble finding a combination of "Inherit default layout" settings that works with the main semantic group wrapper.

I think it's because of the issues described in https://github.com/WordPress/gutenberg/issues/29983. TT1 Blocks always uses its Post Content block inside of a Group block, so that it has a main semantic wrapper around everything. Combinations of settings here don't replicate what we'd had before:

Default, nothing set to "Inherit default layout" Post Content set to "Inherit default layout" Group Container set to "Inherit default layout" Group Container and Post Content set to "Inherit default layout"
Screen Shot 2021-03-22 at 11 36 27 Screen Shot 2021-03-22 at 11 41 21 Screen Shot 2021-03-22 at 11 42 57 Screen Shot 2021-03-22 at 11 42 57

The second one, Post Content set to "Inherit default layout", is the closest, but the Post Title, Separator, Post Meta, and Comments are also supposed to be wide width, and I don't think we can do that without them living inside a group container that inherits the default layout.

I suppose something like this would work, but it seems overkill:

Mockup Description
Screen Shot 2021-03-22 at 11 47 05 A: main Group container, with no layout settings.
B: A nested Group, set to inherit default layout.
C: The post content block, set to inherit default layout.
D: A nested Group, set to inherit default layout.

And even in that option, you'll notice that the post title ("Widths") is standard width instead of its configured wide width, and the separator below it is full-width instead of its configured wide width. So I'm not sure what's going on there.

youknowriad commented 3 years ago

@kjellr Happy to take a look, do we have a demo site or something to check how things are expected to look?

youknowriad commented 3 years ago

It seems there was a recent regression in the layout affecting the post editor, I'm fixing it here https://github.com/WordPress/gutenberg/pull/30093

kjellr commented 3 years ago

@kjellr Happy to take a look, do we have a demo site or something to check how things are expected to look?

I was just testing locally, but in general, the alignments should match the original Twenty Twenty-One theme. So here's what that page looks like there, for reference:

Screen-Shot-2021-03-22-at-12 28 26

youknowriad commented 3 years ago

@kjellr I pushed a fix to the single template which I think do what you want unless I'm missing something?

scruffian commented 3 years ago

I think you want to set inherit layout on every template, and then align wide on the children. I notice that there are some unnecessary alignfull attributes in there too...

kjellr commented 3 years ago

I pushed a fix to the single template which I think do what you want unless I'm missing something?

It generally does — I noticed that template was missing the main wrapper though. I've added in 86fb5f8. This is the solution I described at the bottom of my comment here though:

Mockup Description
Screen Shot 2021-03-22 at 11 47 05 A: main Group container, with no layout settings.
B: A nested Group, set to inherit default layout.
C: The post content block, set to inherit default layout.
D: A nested Group, set to inherit default layout.

It works, but it's 4 group blocks where we used to have just one. Maybe that's ok, but it seems pretty complicated?

I'm also still seeing some of the alignments act a little wonky in the editor:

Current Expected
current expected

The separator width looks like a theme CSS issue, but the site header and site title should both also be wide.

youknowriad commented 3 years ago

It works, but it's 4 group blocks where we used to have just one. Maybe that's ok, but it seems pretty complicated?

It sounds to me that the complication comes from the fact that we want post content's content to allow full width... which means "post-content" can not be wrapped in any div that defines a constrained width.

youknowriad commented 3 years ago

For the alignments not applied properly in the editor. It seems there's something going on with the columns block, it doesn't seem to get the data-align attribute on the wrapper when applying an alignment. I'm not sure why exactly, I'll have to debug that further.

kjellr commented 3 years ago

It sounds to me that the complication comes from the fact that we want post content's content to allow full width... which means "post-content" can not be wrapped in any div that defines a constrained width.

Yeah, exactly. I expect this scenario to be relatively common. Themes without sidebars usually do want full-width items to appear full width, and they (currently at least) need a wrapper div to implement a semantic main tag. And it's also pretty common for post headers and footers to use a layout/width that's also used by the post content.

For the alignments not applied properly in the editor. It seems there's something going on with the columns block, it doesn't seem to get the data-align attribute on the wrapper when applying an alignment. I'm not sure why exactly, I'll have to debug that further.

Thank you!

youknowriad commented 3 years ago

The alignments bugs should be fixed with this PR https://github.com/WordPress/gutenberg/pull/30099

carolinan commented 3 years ago

Can you share the block markup that you are testing with?

carolinan commented 3 years ago

Please remove the remaining --wp--custom--responsive--aligndefault-width at https://github.com/WordPress/theme-experiments/blob/master/tt1-blocks/assets/css/blocks.css#L187 There is a max-width set for the separator, that overrides the alignwide from theme.json.

scruffian commented 3 years ago

The problem with these separator widths removed is that the 100px from theme.css in Gutenberg overrides all the separators so they are all 100px. I think the only way round this is to not use theme styles - removing add_theme_support( 'wp-block-styles' );. What do y'all think?

kjellr commented 3 years ago

removing add_theme_support( 'wp-block-styles' );. What do y'all think?

Does that have negative effects elsewhere? I think in general we do want those styles, right?

scruffian commented 3 years ago

Does that have negative effects elsewhere?

Probably yes, but I can't think of another solution

kjellr commented 3 years ago

I see what you mean. 😞

I just pushed a few additional template updates so that they all use the method from https://github.com/WordPress/theme-experiments/pull/236/commits/b2fa32b8280aab58025973e73c7977d366ccd45a. It's a bunch of extra Group blocks, but it works for now and we can always revise later on.

Everything else appears to be working ok in this PR, so I suggest we try to sort out the separators issue separately? That way at least alignments will work again for folks.

carolinan commented 3 years ago

Adding these group blocks -as in understanding they need them- will be complicated for users when they create their own layouts

kjellr commented 3 years ago

Yeah, I don't think it's a workable long-term solution. But this brings the theme up to date with Gutenberg in the near term, while we sort that out.

fklein-lu commented 3 years ago

It sounds to me that the complication comes from the fact that we want post content's content to allow full width... which means "post-content" can not be wrapped in any div that defines a constrained width.

There are two distinct scenarios here.

The first is editing a template.

In that context it makes sense that containers cannot have blocks that are larger than them. That's the same as when writing HTML: full width means the maximum size is that of the container.

The second is dealing with post content.

The content of a post will always be narrow, because wide paragraphs are unreadable. But still you would want to have images that are larger than the text.

That's a common pattern. Plenty of sites like the New York Times, and even the original Bosco theme have supported for years.

In that case it is inevitable that the wide or large images "break out" of their container. Usually with some clever CSS.

One scenario would be that wide images break out to the right. With full-width breaking out on both sides, but only up until a point.

post-content-wide-full

But let's consider that the rest of the site looks like this, with the navigation wider than the post content, and the header and footer full-width of the browser:

site-wide-full

While I do like the approach taken by Gutenberg, it's not as easy as the current implementation. Content width, wide with, and full-width in the context of editing an entire template is not the same as when dealing with post content.

carolinan commented 3 years ago

Wide content only breaking out on one side -unless aligned to that side with the block alignment option, is not what I would prefer.

youknowriad commented 3 years ago

There are two distinct scenarios here.

I do agree with this and it's exactly why the "layout" config applies to the post editor and not the site editor. In the site editor, you define the containers that support it explicitly

Wide content only breaking out on one side -unless aligned to that side with the block alignment option

I think what's important is that the expectations should be set properly here.

This doesn't mean we can't support these side alignments, but these are use cases for different kind of layouts that are different from the "default" layout (which is the only one supported now). It can be achieved later with things like "grid layouts and alignments" or other kind of opinionated layouts.

The Layout API is not yet in a place where we can open it more broadly though. In the first days, we'd only support the default one and I'd like to discourage folks from changing the "meaning"/"semantics" of these things with CSS. Doing that just means potential future breakage.

fklein-lu commented 3 years ago

I do agree with this and it's exactly why the "layout" config applies to the post editor and not the site editor. In the site editor, you define the containers that support it explicitly

Right, but re-using the same terms means that this will lead to confusion.

Why not allow theme developers to define their own set of widths, rather then staying with "wide" and "full"? Plus a box to define the width manually as it is now the case.

In that sense the layout controls would work the same as the font size controls for example. Either use a pre-defined width, or define it yourself.

That seems to me a lot clearer then reusing existing terminology and interface elements from the post editor. Plus that "Inherit default layout" toggle is weird from an interface perspective, and it's not super clear what the default actually is. The interface doesn't tell you that at all.

youknowriad commented 3 years ago

Why not allow theme developers to define their own set of widths, rather then staying with "wide" and "full"? Plus a box to define the width manually as it is now the case.

Defining your own set of widths is very related to Grid block proposal as well. It's not something that impact the container block only, it impacts controls of the children blocks too that become dependent on the parent. It's a new kind of UI and behavior that doesn't exist in any of the editors. The goal for the first iteration of the layout proposal is to actually fix what already exists (wide/full alignements which a lot of blocks already support) and build the framework pieces that allow more advanced layouts like you suggest. There's time for everything, we can't build everything in one ago, we iterate towards it.

kjellr commented 3 years ago

While we sort out those larger questions, would someone mind giving this PR a review? At the moment, TT1 Blocks looks pretty broken while running the trunk branch of Gutenberg. Even if we adjust this system later on, I think it's worthwhile to get this working for now.

kjellr commented 3 years ago

Thanks! @youknowriad do you know when is this alignments update will be published to the plugin repo? We can make sure to push a new version of TT1 Blocks to the theme repository when that happens.

youknowriad commented 3 years ago

yes, next Wednesday

kjellr commented 3 years ago

Awesome, thank you!