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.

Sign up for GitHub

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

Jump to bottom

[Experiment] Patterns: Allow the heading block and the button block to be partially synced #56704

Closed
wants to merge 2 commits into from
Closed

[Experiment] Patterns: Allow the heading block and the button block to be partially synced #56704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5924426
Allow the heading block and the button block to be partially synced
kevin940726 Dec 1, 2023
2c014f6
Update lib/experimental/blocks.php
kevin940726 Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions lib/experimental/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,17 @@ function gutenberg_render_block_connections( $block_content, $block, $block_inst
// Allowlist of blocks that support block connections.
// Currently, we only allow the following blocks and attributes:
// - Paragraph: content.
// - Heading: content.
// - Button: text.
Comment on lines +103 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure these comments are necessary, seeing how the same thing is listed below in an easy to read format. 😄

// - Image: url.
$blocks_attributes_allowlist = array(
'core/paragraph' => array( 'content' ),
'core/heading' => array( 'content' ),
'core/button' => array( 'text' ),
Copy link
Contributor

@talldan talldan Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, button blocks don't seem to be working correctly. When they have an override they're missing the <div class="wp-block-button"> wrapper.

It's because the code below does this:

$tag_name     = $tags->get_tag();
$markup       = "<$tag_name>$custom_value</$tag_name>";
$updated_tags = new WP_HTML_Tag_Processor( $markup );
$updated_tags->next_tag();
return $updated_tags->get_updated_html();

So it considers the updated html of the block to just be the tag that it's updating and disregards any markup before or after that.

Instead it needs to update the $original_tags and get_updated_html from that, but I don't think it's supported yet 🤔

It is mentioned in the block bindings tracking issue - #54536.

Support to mutate HTML to change the inner content of an element. Until we have a set_inner_markup function, we can manually mutate the HTML for our experiments.

I guess we need to drop button support until then.

Image also doesn't work, as the code only updates tag content, not attribute values, so you end up with <image>$url</image> and it would also strip the image block <figure> wrapper, so I think that should be removed too.

Copy link
Member

@gziolo gziolo Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@SantosGuillamot opened #57249 that updates the PHP implementation for block bindings that should fix the handling for the Heading and Button blocks. It was extracted from the full prototype for Custom Fields: #56867.

'core/image' => array( 'url' ),
);

// Whitelist of the block types that support block connections.
// Currently, we only allow the Paragraph and Image blocks to use block connections.
if ( ! in_array( $block['blockName'], array_keys( $blocks_attributes_allowlist ), true ) ) {
return $block_content;
}
Expand Down Expand Up @@ -168,14 +171,14 @@ function gutenberg_render_block_connections( $block_content, $block, $block_inst
continue;
}

$tags = new WP_HTML_Tag_Processor( $block_content );
$found = $tags->next_tag(
array(
// TODO: In the future, when blocks other than Paragraph and Image are
// supported, we should build the full query from CSS selector.
'tag_name' => $block_type->attributes[ $attribute_name ]['selector'],
)
);
$selectors = explode( ',', $block_type->attributes[ $attribute_name ]['selector'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to add a comment above this saying something like:

// Support comma-separated selectors by exploding them into an array.

I guess it's obvious in the context of this PR given it mentions the various heading block tags, but it might not be so obvious if you're just looking at this file on its own wondering why it works this way.

$found = false;
while ( ! $found && count( $selectors ) > 0 ) {
$tags = new WP_HTML_Tag_Processor( $block_content );
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, it might be worth a comment saying something like:

// Creating a new tag processor on every loop resets the search to the start.

// TODO: In the future, to support connecting more block attributes,
// we should build the full query from CSS selector.
$found = $tags->next_tag( trim( array_shift( $selectors ) ) );
}
Copy link
Member

@dmsnell dmsnell Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we could invert this loop and avoid the confusion @talldan wants to clarify, while improving performance.

  • iterate through all the tags until
  • we find a tag matching one of the selectors

this assumes that there are likely more tags than selectors, many more, and so the cost of checking each selector at each tag will be cheaper than checking every tag at each selector. additionally, we should make it pretty explicit here in a comment that the only selectors we currently support are tag names.

$selectors = array();
foreach ( explode( ',', $block_type->attributes[ $attribute_name ]['selector'] ) as $tag_name ) {
	$selectors[] = trim( strtoupper( $tag_name ) );
}

$tags = new WP_HTML_Tag_Processor( $block_content );
while ( $tags->next_tag() ) {
	$tag_name = $tags->get_tag();
	
	if ( ! in_array( $tag_name, $selectors, true ) ) {
		 continue;
	}
	
	$markup       = "<$tag_name>$custom_value</$tag_name>";
	$updated_tags = new WP_HTML_Tag_Processor( $markup );
	$updated_tags->next_tag();

	// Get all the attributes from the original block and add them to the new markup.
	$names = $tags->get_attribute_names_with_prefix( '' );
	foreach ( $names as $name ) {
		 $updated_tags->set_attribute( $name, $tags->get_attribute( $name ) );
	}

	return $updated_tags->get_updated_html();
}

return $block_content;

if ( ! $found ) {
return $block_content;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/button/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@
"width": true
}
},
"__experimentalSelector": ".wp-block-button .wp-block-button__link"
"__experimentalSelector": ".wp-block-button .wp-block-button__link",
"__experimentalConnections": true
},
"styles": [
{ "name": "fill", "label": "Fill", "isDefault": true },
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/heading/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@
}
},
"__unstablePasteTextInline": true,
"__experimentalSlashInserter": true
"__experimentalSlashInserter": true,
"__experimentalConnections": true
},
"editorStyle": "wp-block-heading-editor",
"style": "wp-block-heading"
Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/hooks/pattern-partial-syncing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ const {
/**
* Override the default edit UI to include a new block inspector control for
* assigning a partial syncing controls to supported blocks in the pattern editor.
* Currently, only the `core/paragraph` block is supported.
*
* @param {Component} BlockEdit Original component.
*
Expand Down
2 changes: 2 additions & 0 deletions packages/patterns/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ export const PATTERN_SYNC_TYPES = {
// TODO: This should not be hardcoded. Maybe there should be a config and/or an UI.
export const PARTIAL_SYNCING_SUPPORTED_BLOCKS = {
'core/paragraph': { content: __( 'Content' ) },
'core/heading': { content: __( 'Content' ) },
'core/button': { text: __( 'Text' ) },
};
Loading