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

FSE: Make nested layout containers expand the full width of their parent #33501

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

@MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Jul 16, 2021

Description

When we have nested layout containers, the child container is being affected by the parent's max-width rules. This makes it mandatory for the child container to use align full for the grand children to be able to benefit from alignment rules

How has this been tested?

Using emptytheme, I defined a layout on theme.json:

{
	"settings": {
		"layout": {
			"contentSize": "640px",
			"wideSize": "1100px"
		}
	}
}

I then created a template with the following markup (I just edited singular for this):

<!-- wp:group {"tagName":"main","layout":{"inherit":true}} -->
<main class="wp-block-group post-content">
<!-- wp:post-content {"layout":{"inherit":true}} /-->
</main>
<!-- /wp:group -->

Without align full, the post-content block will never be able to have full or wide width children. With this PR, the align full rule is no longer needed

Screenshots

Before this PR, with alignfull on the post-content block:

Screenshot 2021-07-16 at 12 56 42

After this PR, without align full.
Screenshot 2021-07-16 at 12 57 01

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 19, 2021

Thanks for pursing this, it's a hard issue to solve.

With the proposed approach here, I see a number of issues:

  • It's impossible to say float an element inside a container, the solution we used to apply for this is to insert a group inside a container and insert a left aligned block inside the centered group. How would you solve this kind of layouts and similar layouts (centered group inside a group)
  • This is going to break existing sites relying on the current behavior, we should be careful about changing the default behaviors.
  • the container class doesn't necessarily mean a group block that has a regular flow layout, while this is the only layout for now, introducing alternative layouts is planned see #33359, so for instance if I add a "navigation block" (or any block that could rely on a "flex layout"), it shouldn't be full width by default right?
  • The main issue for me though is the inconsistency between blocks, some will be full width and others not and it can be confusing for users.
@scruffian
Copy link
Contributor

@scruffian scruffian commented Jul 19, 2021

This is going to break existing sites relying on the current behavior, we should be careful about changing the default behaviors.

Since hasLayout is a brand new feature I'm not sure that's a concern here is it?

@pbking
Copy link
Contributor

@pbking pbking commented Jul 19, 2021

The addition class may seem superfluous but follows the alignment rules more specifically. The additional 'align full' class, I believe, is appropriate in the provided scenario. The intent is explicit.

An alternate is to not inherit the layout from the top-level group but exclusively from the post-content.

<!-- wp:group {"tagName":"main"} -->
<main class="wp-block-group">
<!-- wp:post-content {"layout":{"inherit":true}} /-->
...

I could see either solution being appropriate for different scenarios. But I'm keen on the idea that the default behavior effects all contained blocks the same regardless of the container-ness of the contained block.

What I don't expect (noted in the originating blockbase issue) is that the two environments (editor/view) don't agree. So indeed there is still a problem to resolve, one way or the other!

@carolinan
Copy link
Contributor

@carolinan carolinan commented Jul 19, 2021

I did not understand why needing to add the alignfull to the child is an issue.

@pbking
Copy link
Contributor

@pbking pbking commented Jul 19, 2021

One thing I just realized poking further is that while using the post/page editor there’s no knowledge of what alignment restrictions are imposed outside of the post-content block being edited. So the editor will always act as though there are no restrictions. This is why the behavior is different in the originating issue; alignment restrictions are imposed OUTSIDE of the editor and those only come into account when rendering the VIEW not the EDITOR.

While I'm not sure what the overarching fix for this situation should be I think that in the meantime it would simply be better practice to NOT impose any layout restrictions on blocks that contain the post-content block in the theme block-templates.

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 19, 2021

alignment restrictions are imposed OUTSIDE of the editor and those only come into account when rendering the VIEW not the EDITOR.

This is the reason there's a "layout" config in theme.json, that config applies to the post editor.

@pbking
Copy link
Contributor

@pbking pbking commented Jul 19, 2021

This is the reason there's a "layout" config in theme.json, that config applies to the post editor.

Ah! I see, and that also creates a similar problem.

Consider the theme.json defining layout parameters and in one block-template imposes that layout on the post-content block and on another does not (instead using perhaps a percentage value). The editor imposes the layout restrictions but the view does so differently; another discrepancy.

The alignment of the content in the post editor is based on assumption, not configuration. Which is what is happening in the originating issue: The editor ASSUMES that the content is the TOP LEVEL of any nested containers while the view calculates that layout situation differently.

Adding "alignfull" seems to mask the problem, causing the blocks to layout as expected in both environments, but is only necessary in ONE of the environments (the one that's already behaving correctly based on the rules its given [the view], not the one that's not following the rules [the editor]).

@scruffian
Copy link
Contributor

@scruffian scruffian commented Jul 20, 2021

I did not understand why needing to add the alignfull to the child is an issue.

My issue with it is that it's not very intuitive. If I add a block with a default layout then I'll learn how it behaves from experimenting with it. Then if I add a child which also has default layout enabled, the behaviour of the child will be different from its parent. It wouldn't be clear to me as a user that the solution to this was to change the alignment of the child block.

@pbking
Copy link
Contributor

@pbking pbking commented Jul 20, 2021

the behaviour of the child will be different from its parent

But this is because the parent is acting upon ALL children the same be it another container, an image, or a block of text. Making an exception for "nested containers expressing layout" is what seems confusing to me. If I tell a container "constrain all of your children to 600px, except those who say they are WIDE or FULL width" then that is what I believe should happen.

I really think the whole issue at hand has stemmed from the block editor assuming the post-content would always be the top-level container, and always have 'inherit layout' defined. It was confusing in this use-case because the editor and view were doing different things for some of the blockbase templates because, in that case, the post-content was NOT the top-level "inherit layout" container.

So I DON'T believe the fix is to apply 'full width' to post-content in order to behave as we expect. While that is A way to fix it I believe it should be avoided if possible. I believe the fix is to ensure that UNTIL/UNLESS the block editor can discern and handle the layout configuration of the post-content more explicitly that we instead always ensure that the post-content block fits the editors expectations.

@devfle
Copy link
Contributor

@devfle devfle commented Jul 20, 2021

Is there a way to disable this whole feature without removing the theme.json from the theme? Exactly this case unfortunately breaks all my patterns...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants