Pattern block: remove wrapping div #36090
Conversation
|
Size Change: -72 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
I'll let other folks review the code, but I can confirm that this works as expected. When I open up the site editor, the pattern block is entirely transparent, and it just shows me the blocks that the pattern contains:
It works correctly across multiple templates, and only stops syncing with the pattern when I manually edit it in the site editor. Seems pretty solid to me. 👍 |
| } | ||
| }, [ selectedPattern?.blocks ] ); | ||
|
|
||
| const props = useInnerBlocksProps( useBlockProps(), {} ); | ||
| const props = useBlockProps(); | ||
|
|
||
| return <div { ...props } />; |
There was a problem hiding this comment.
This works, it makes the "pattern block" a temporary block (only used to load the template to the editor) but there's a gotcha.
Previously, changes were only saved if the pattern itself was modified, now the pattern will stop being used as soon as the template (or template part) where it's used is modified.
I'm fine with the change if you're fine with this behavior.
There was a problem hiding this comment.
Personally I would prefer to keep the previous behaviour as it keeps the templates more flexible. Could we use an invisible wrapper, like React.Fragment?
There was a problem hiding this comment.
There's also the possibility of using a <div> with display: contents, although there's an accessibility bug with it in Safari at the moment that may make it a non-starter for the time being. https://caniuse.com/css-display-contents
There was a problem hiding this comment.
Previously, changes were only saved if the pattern itself was modified, now the pattern will stop being used as soon as the template (or template part) where it's used is modified.
Generally, this seems fine to me. Given our current use cases at least, the translation has already been done at that point. Can someone think of a use case where that wouldn't be ok?
There was a problem hiding this comment.
The use case would be that the user changes their site language after they edit a template. With this change, once a user selects a site language they will need to update their templates with new translations.
There was a problem hiding this comment.
I would prefer to keep the previous behaviour as it keeps the templates more flexible. Could we use an invisible wrapper, like React.Fragment?
I will try this.
There was a problem hiding this comment.
I gave this a try and <Fragment> + inner blocks does not have the desired effect.
Previously, changes were only saved if the pattern itself was modified
I also tested a few more scenarios, and I don't think the templates are more that much more flexible with the previous behavior. Take the following example on trunk:
- Inside the site editor, select the pattern or any of its inner blocks.
- Make a separate change to the template (unrelated to the pattern)
- Save
This will still cause the pattern to no longer from the pattern file.
With this PR, you can still make changes to other template parts, and the pattern will still pull from the file.
The use case would be that the user changes their site language after they edit a template.
How common is this? The problems of alignment introduced by adding this additional wrapper are significant, so would love to find a solution here that doesn't introduce it.
There was a problem hiding this comment.
The use case would be that the user changes their site language after they edit a template.
Yeah, I think in general that use case seems like an edge case to me. As @jffng noted, the approach in this PR eliminates a somewhat prominent layout issue, so I'd be happy to settle for what's in this PR and consider anything beyond this behavior to be an enhancement or bug fix later on.
There was a problem hiding this comment.
I agree that the approach in this PR is good enough for now and an improvement over master.
Also, I don't really know if it's applicable here, and it's certainly beyond the scope of this PR, but just to throw the idea out there: I wonder if we could find a solution via Shadow DOM? Browser support looks viable.
There was a problem hiding this comment.
Happy to ship with this compromise for now
|
I'm happy to approve this PR if we all are okay with the behavior @youknowriad described - #36090 (comment)
|
|
Yes, I think that behavior is fine for now. The layout issue this PR solves is a more visible issue than that use case. We can open up a followup issue to continue thinking through other solutions. |
|
Thanks everyone, I'll merge this and open a follow up issue to track how we can persist the pattern when the template is edited. |
andrewserong
added
the
[Type] Bug
Description
The Pattern block contains logic that inserts the contents of the referenced pattern inside of a wrapping container. This container is replaced with a Group upon block selection / editing. We added a wrapping div on the front-end to make the alignment consistent between editor and front-end.
Working on 2022, I found that the additional wrapper can cause unintended issues with layouts. This PR removes the logic in favor of simply placing the patterns contents into the block tree without an additional wrapper.
The block's behavior should be unchanged — if no edits have been made, it still pulls from the pattern file.
How has this been tested?
inc/patterns/footer-default.phpfile.Screenshots
Types of changes
Technical iteration on a new block
Checklist:
*.native.jsfiles for terms that need renaming or removal).