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

Update TT1 Blocks to use Gutenberg alignments #236

Merged
merged 10 commits into from
Mar 25, 2021

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Mar 18, 2021

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 kjellr added the bug Something isn't working label Mar 18, 2021
@kjellr kjellr requested review from youknowriad and scruffian March 18, 2021 15:18
@kjellr kjellr self-assigned this Mar 18, 2021
@@ -9,7 +9,7 @@
<!-- wp:query-loop -->
<!-- wp:post-title {"isLink":true} /-->

<!-- wp:post-content {"align":"full"} /-->
<!-- wp:post-content /-->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this in place, posts on the front-end extended beyond the edges of the screen. So I've removed it for now. Should probably re-evaluate once I can get the front-end alignments working right in general though.

Copy link
Contributor

@youknowriad youknowriad Mar 19, 2021

Choose a reason for hiding this comment

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

you should add { layout: { inherit: true } } to all post content in the templates if they are at the root of templates..

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

you can also remove the "align-wide" theme flag if it's defined in functions.php

@@ -3,7 +3,7 @@
<!-- wp:group {"tagName":"main", "align":"full"} -->
<main class="wp-block-group alignfull">
<div class="wp-block-group__inner-container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question, were we not meant to remove the inner containers too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safe to remove yes, the deprecation probably triggers here making sure everything works with or without them but yes, it's better to remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I've removed these in af20e47.

@kjellr
Copy link
Collaborator Author

kjellr commented Mar 22, 2021

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 WordPress/gutenberg#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
Copy link
Contributor

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

@youknowriad
Copy link
Contributor

It seems there was a recent regression in the layout affecting the post editor, I'm fixing it here WordPress/gutenberg#30093

@kjellr
Copy link
Collaborator Author

kjellr commented Mar 22, 2021

@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
Copy link
Contributor

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

@scruffian
Copy link
Collaborator

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
Copy link
Collaborator Author

kjellr commented Mar 22, 2021

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
Copy link
Contributor

youknowriad commented Mar 22, 2021

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
Copy link
Contributor

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
Copy link
Collaborator Author

kjellr commented Mar 22, 2021

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
Copy link
Contributor

The alignments bugs should be fixed with this PR WordPress/gutenberg#30099

@carolinan
Copy link
Collaborator

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

@carolinan
Copy link
Collaborator

carolinan commented Mar 23, 2021

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
Copy link
Collaborator

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
Copy link
Collaborator Author

kjellr commented Mar 23, 2021

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
Copy link
Collaborator

Does that have negative effects elsewhere?

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

@kjellr
Copy link
Collaborator Author

kjellr commented Mar 23, 2021

I see what you mean. 😞

I just pushed a few additional template updates so that they all use the method from b2fa32b. 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
Copy link
Collaborator

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

@kjellr
Copy link
Collaborator Author

kjellr commented Mar 23, 2021

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
Copy link

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
Copy link
Collaborator

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
Copy link
Contributor

youknowriad commented Mar 24, 2021

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.

  • Wide means wide in both sides and you just define the width
  • Same for full.

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
Copy link

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
Copy link
Contributor

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
Copy link
Collaborator Author

kjellr commented Mar 25, 2021

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.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This looks good to me. We can iterate if we find things we missed in the refactor.

@kjellr kjellr merged commit c234d5d into master Mar 25, 2021
@kjellr kjellr deleted the update/tt1-blocks-alignments branch March 25, 2021 15:21
@kjellr
Copy link
Collaborator Author

kjellr commented Mar 25, 2021

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
Copy link
Contributor

yes, next Wednesday

@kjellr
Copy link
Collaborator Author

kjellr commented Mar 25, 2021

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants