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

Try: Custom block insertion hooks #45098

Open
wants to merge 4 commits into
base: try/example-block-hooks
Choose a base branch
from

Conversation

joshuatf
Copy link
Contributor

Changes proposed in this Pull Request:

This PR uses a custom callback for the before/after block visitors and introduces custom hooks to allow insertion of blocks. It differs in the current block hooks in that:

  • We can insert the entire block and block attributes in a single hook
  • We have access to the entire anchor block and attributes for conditional insertion
  • Inserted blocks can also be hooked onto to further insert blocks
Screenshot 2024-02-23 at 5 30 51 PM Screenshot 2024-02-23 at 5 30 46 PM

How to test the changes in this Pull Request:

  1. Install the latest WP 6.5 beta 2 release
  2. Make sure the product editor is enabled under WooCommerce -> Settings -> Advanced -> Features.
  3. Navigate to Products -> Add new
  4. Note the inserted blocks

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@joshuatf joshuatf requested review from ockham and a team February 23, 2024 22:36
@joshuatf joshuatf self-assigned this Feb 23, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 23, 2024
Copy link
Contributor

Hi @ockham, @woocommerce/mothra

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

// Parse these first so markup is generated from all inner blocks.
$first_chunk = $this->insert_blocks( $parsed_inserted_block, 'first_child', $hooked_blocks, $context );
$last_chunk = $this->insert_blocks( $parsed_inserted_block, 'last_child', $hooked_blocks, $context );
array_unshift( $parsed_inserted_block['innerContent'], $first_chunk );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs further investigation to make sure this won't break things. I suspect this could be problematic depending on existing chunks in innerContent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to prepend/append the hooked block to innerBlocks rather than innerContent:

Suggested change
array_unshift( $parsed_inserted_block['innerContent'], $first_chunk );
array_unshift( $parsed_inserted_block['innerBlocks'], $first_chunk );

Additionally, you also need to insert a null in the corresponding position in innerContent, so it should actually read

Suggested change
array_unshift( $parsed_inserted_block['innerContent'], $first_chunk );
array_unshift( $parsed_inserted_block['innerBlocks'], $first_chunk );
array_unshift( $parsed_inserted_block['innerContent'], null );

The way it works is that innerContent can have items that are either HTML strings, or null to mark a position where an item from innerBlocks needs to be interpolated. E.g.

$group_block_with_some_inner_block = array(
	'blockName' => 'core/group',
	'attrs'     => array(),
	'innerBlocks' => array( $some_inner_block ),
	'innerContent' => array(
		'<div class="wp-block-group">',
		null,
		'</div>'
	),
);

Copy link
Contributor

@ockham ockham Feb 27, 2024

Choose a reason for hiding this comment

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

This is still not the full picture BTW if you want to account for some subtleties: As the Group block example demonstrates, you might not actually want to put the null marker for first_child in the very first position in innerContent (or the one for last_child in the very last one), as you might want to account for the wrapping HTML:

$group_block_with_some_inner_block = array(
	'blockName' => 'core/group',
	'attrs'     => array(),
	'innerBlocks' => array( $hooked_first_child, $some_inner_block ),
	'innerContent' => array(
		null, // $hooked_first_child is going to be rendered outside the wrapping div :/
		'<div class="wp-block-group">',
		null,
		'</div>'
	),
);

Instead, you'll likely want to find the first (last) non-HTML location in innerContent. For the original Block Hooks implementation (in GB's WP 6.4 compat shim), we ended up with the following logic: https://github.com/WordPress/gutenberg/blob/98552b2486b52e4dd3c90775c2a029b95e64253c/lib/compat/wordpress-6.4/block-hooks.php#L157-L182

Copy link
Contributor

Test Results Summary

Commit SHA: 73fce83

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests2870121030913m 8s

⚠️ Warning

Please address the following issues prior to merging this pull request:
  • FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants