-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add synced patterns support #75
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I noticed that gutenberg does is that, it adds this paragraph block with the text Ignore this, as it was me who added a paragraph saying that and forgot about it. Always check your blocks, folks.This is a custom pattern
to every block as an inner block. Should this be something worth filtering out, and offering a filter that could reverse it if someone doesn't want it?
It looks good to me. Would want to also get @alecgeatches's take on this PR.
Analytics::record_usage(); | ||
|
||
if ( isset( $filter_options['exclude'] ) && isset( $filter_options['include'] ) ) { | ||
return new WP_Error( 'vip-block-data-api-invalid-params', 'Cannot provide blocks to exclude and include at the same time', [ 'status' => 400 ] ); | ||
} | ||
|
||
$this->post_id = $post_id; | ||
// Temporarily set global $post. | ||
$previous_global_post = $post; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure I understand this, why is this needed? Would be worth documenting the reasoning behind this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved the comment in 0c1b48a, thanks!
README.md
Outdated
if ( 'core/gallery' !== $block_name ) { | ||
return $inner_blocks; | ||
} | ||
|
||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is just an example and would work, but it feels backwards to me. How about the inverse ordering:
if ( 'core/gallery' !== $block_name ) { | |
return $inner_blocks; | |
} | |
return []; | |
if ( 'core/gallery' === $block_name ) { | |
return []; | |
} | |
return $inner_blocks; |
@@ -1309,11 +1341,11 @@ Modify or add attributes to a block's output in the Block Data API. | |||
* | |||
* @param array $sourced_block An associative array of parsed block data with keys 'name' and 'attributes'. | |||
* @param string $block_name The name of the parsed block, e.g. 'core/paragraph'. | |||
* @param string $post_id The post ID associated with the parsed block. | |||
* @param string $block The result of parse_blocks() for this block. | |||
* @param int $post_id The post ID associated with the parsed block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing these!
// inside a dynamic block's render callback function. | ||
// | ||
// https://github.com/WordPress/WordPress/blob/6.6.1/wp-includes/class-wp-block.php#L517 | ||
$parent_block = WP_Block_Supports::$block_to_render; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we wrap this in some sort of safety method in case it becomes private or changes names in the future? Like an isset()
/property_exists()
check. I think we can just bail if we don't see the variable we expect, and we'll see that synced pattern tests are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works really well! I spent some time reviewing the code and debugging locally to watch how it works.
I left a couple of notes, but because there's nothing major I'll just fix those in this branch and merge. Thank you Chris!
bc8ea11
Description
Add support for synced patterns (previously known as "reusable blocks"). Synced patterns are implemented as a dynamic block (
core/block
) and currently result in a dead end in the API output:However that
ref
is a pointer to awp_block
post, where the blocks (and potential block data) is stored. This PR adds support for loading those blocks:Setting aside tests and comments, the diff here is only about 150 LOC. Its primary features are:
CoreBlock
class to hook into the parser. These hooks are documented in the README.The
CoreBlock
does some fairly advanced filtering but is well-documented in the comments. Likewise, the tests are fairly extensive but feel free to suggest additional cases.