i18n: Add pattern block #33217
i18n: Add pattern block #33217
Conversation
scruffian
added
the
Internationalization (i18n)
scruffian
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +272 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
scruffian
force-pushed
the
add/pattern-block
branch
from
bc428d9
to
1182dfb
Compare
|
I updated this as suggested by @mcsf. I'd love to get more thoughts on it. |
mtias
added
the
Needs Technical Feedback
|
A few things I think are needed:
|
|
Thanks for the great feedback @mcsf. I updated this to do a couple of things:
I'm not sure if this combination of replaceBlock and replaceInnerBlocks is the right tool for the job but when all you |
There was a problem hiding this comment.
It's remarkable how such a small amount of code gets us so far. This works reasonably, considering.
There is some potential for hiccoughs due to the way that an entire subtree of blocks can be silently swapped out upon user selection. For instance, if one has the List View enabled and uses it to select something inside the Pattern block, the async rendering creates a strange delayed effect — it doesn't break anything, it's just a little clunky to see. I don't think this is an issue in itself, but cc @youknowriad for good conscience. :)
Gravacao.do.ecra.2021-07-27.as.17.47.38.mov
| save: () => { | ||
| return <InnerBlocks.Content />; | ||
| }, |
There was a problem hiding this comment.
Thanks to the logic in PatternEdit to replace the block with its contents, I think we can remove save entirely.
As far as I can tell, there should never be a case where there are inner blocks to serialise for this block. I even think that allowing Pattern to serialise any kind of content here opens the door for bugs where the edit side wants to use the freshest pattern from __experimentalGetParsedPattern while save has saved something else to the database.
What do you think?
There was a problem hiding this comment.
Yeah that makes sense.
|
@scruffian: don't forget to add fixtures for the block, per the failing integration tests. |
scruffian
force-pushed
the
add/pattern-block
branch
from
cc75e9c
to
1d8353a
Compare
|
I think I have addressed all the comments here, so this is ready for another review. Thanks again for looking... |
|
Should the theme name (which is not a default theme either) be included in the fixtures? |
| @@ -0,0 +1 @@ | |||
| <!-- wp:pattern {"slug":"quadrat/search-results"} /--> | |||
| $slug = $attributes['slug']; | ||
| if ( class_exists( 'WP_Block_Patterns_Registry' ) && WP_Block_Patterns_Registry::get_instance()->is_registered( $slug ) ) { | ||
| $pattern = WP_Block_Patterns_Registry::get_instance()->get_registered( $slug ); | ||
| return do_blocks( $pattern['content'] ); |
There was a problem hiding this comment.
In the editor there's a "div" wrapper but there's none here, meaning that the "alignments" won't work the same way between the editor and the frontend. I think it's safer to add a wrapper here as well to unify both behaviors.
There was a problem hiding this comment.
Actually, wait here, I'm still forming some ideas on a related comment :)
There was a problem hiding this comment.
|
|
||
| // Run this effect when the block, or any of its InnerBlocks are selected. | ||
| // This replaces the Pattern block wrapper with the content of the pattern. | ||
| // This change won't be saved unless further changes are made to the InnerBlocks. |
There was a problem hiding this comment.
If the user changes something else in the template outside the pattern block, it will also save a "copy" of the pattern in the template and not reference the pattern anymore, is this intended?
There was a problem hiding this comment.
That's the way that patterns work in general, so I think yes.
There was a problem hiding this comment.
I misread you here, sorry. I think if the user changes the pattern then we should save a copy, but if they change other things in the template the pattern should still be loaded from the file.
jffng
force-pushed
the
add/pattern-block
branch
from
653cc0e
to
31ec373
Compare
jffng
force-pushed
the
add/pattern-block
branch
from
445e857
to
3843b3e
Compare
| } | ||
| }, [ selectedPattern?.blocks ] ); | ||
|
|
||
| const props = useInnerBlocksProps( useBlockProps(), {} ); |
There was a problem hiding this comment.
I am not sure if I implemented this correctly — does anything need to be supplied here?
|
Doesn't converting the Pattern block to a Group change the front-end markup? Patterns (and Reusable Blocks) have no encompassing wrapper when rendered on the front-end. Any wrappers exist solely in the editor for the sake of marking the block boundaries. Or am I misunderstanding what's going on here? |
The markup structure is mirrored between the editor and front-end because the Pattern block's contents are output inside a wrapping div on the front-end: The Pattern block is only converted to a Group if edits are made to the Group or any of its inner blocks. Does that help to clarify? |
|
@jffng Oh, interesting. That's good to know. I suspect the use of a plain |
| $slug = $attributes['slug']; | ||
| if ( class_exists( 'WP_Block_Patterns_Registry' ) && WP_Block_Patterns_Registry::get_instance()->is_registered( $slug ) ) { | ||
| $pattern = WP_Block_Patterns_Registry::get_instance()->get_registered( $slug ); | ||
| return do_blocks( '<div>' . $pattern['content'] . '</div>' ); |
There was a problem hiding this comment.
To avoid styling inconsistencies when converting to a Group, perhaps we should add the Group block's CSS class here:
| return do_blocks( '<div>' . $pattern['content'] . '</div>' ); | |
| return do_blocks( '<div class="wp-block-group">' . $pattern['content'] . '</div>' ); |
There was a problem hiding this comment.
Technically it is not a Group block until it's converted. Not sure how semantic this would be.
There was a problem hiding this comment.
Correct, it's not a Group until it's converted + edited, so I don't think we should add the group block CSS class here.
There was a problem hiding this comment.
Could we make it render an actual Group block, though? In other words, would it be possible to generate and render an actual Group block on the front-end?
There was a problem hiding this comment.
Yes it's possible, an earlier iteration of the PR did that, but the feedback was given that a plain div is better / more semantic, since the Pattern block is not the same thing as a Group block.
This is an example of the default state (no edits made to the Pattern block's contents):
| Site Editor | View |
|---|---|
 |
 |
I do think it is a bit tricky to wrap the head around, but why would it make more sense to output a group on the front-end if the contents haven't been edited?
There was a problem hiding this comment.
I've noticed some themes tend to target Group blocks with styles like additional padding/margin, which they don't apply to plain <div>s. In fact, I've even written Custom CSS like that within the past week or so.
If the Pattern block doesn't always output a Group, then editing it could cause a bunch of spacing to appear/disappear out of nowhere on some themes, which would be quite confusing for users.
There was a problem hiding this comment.
And it's not just basic margin/padding, either. Some themes have special handling for Wide/Full block alignments when inside a Group versus not inside a Group, so this could cause some really bizarre layout shifts in those situations.
There was a problem hiding this comment.
I've noticed some themes tend to target Group blocks with styles like additional padding/margin, which they don't apply to plain
s. In fact, I've even written Custom CSS like that within the past week or so
These seem like reasons for not rendering the Pattern inside a Group. I think the most likely scenario is for a user to not edit this block, since currently its main use cases for are string translation or including a dynamic URL inside templates. If it were wrapped inside a group by default, the risk of side effects from themes with CSS targeting Groups seems higher.
Open to other's reviewers perspectives here!
There was a problem hiding this comment.
Also, this is all new (FSE themes) so we shouldn't base our decisions around practices in existing themes more about best practices.
I'm not sure styling raw groups is a good idea as Group block is meant to be just a div in its essence.
That said, I don't feel strongly here and I don't think it's that important either for a start. Feedback later will help us make the best decision.
|
Is there a definition of what a "Pattern block" is and/or any documentation on this yet? In skimming this PR, it appears that the Pattern block is designed purely for theme development and is not something that a user would actually insert into Block Editor. Is that correct? |
Not yet, where should this go? Maybe in the Block Theme Overview?
I think that's correct, inserting a pattern is still done via the pattern inserter. Knowing this, I think the block category should change to |
|
@ndiego yes, currently themes can include patterns but they cannot directly use them in templates. This starts to pave the way for patterns and templates to be a more fluid system. |
Description
This PR is a proposal to add a pattern block. This will enable block theme developers to do a few things inside their templates:
For more context, see #33192 (comment). This is another approach to that PR.
How has this been tested?
Screenshots
Kapture.2021-10-07.at.14.42.47.mp4
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).