Load supported email editor blocks from supports settings #58966
Conversation
WalkthroughThis change updates the email editor to determine supported blocks using a metadata flag ( Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
lysyjan
force-pushed
the
wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
branch
from
3e6e943 to
68c01ef
Compare
|
Size Change: +1.34 kB (+0.02%) Total Size: 5.95 MB
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/js/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings (1)
4-4: Fix typo in changelog description."alowed" should be "allowed".
-Add loading alowed blocks in the email editor by metadata `supports.email`. +Add loading allowed blocks in the email editor by metadata `supports.email`.packages/php/email-editor/src/Engine/class-email-editor.php (1)
101-101: Consider scoping the admin hook to relevant pages only.The hook registration looks correct, but it will execute on every admin page. Consider adding a condition to only register this hook when the email editor is needed.
- add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_admin_styles' ) ); + if ( $this->is_email_editor_context() ) { + add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_admin_styles' ) ); + }You would need to implement
is_email_editor_context()to check if the current page requires email editor functionality.packages/js/email-editor/src/blocks/utils.ts (1)
9-16: Consider improving type safety and error handling.The implementation logic is correct, but there are opportunities for improvement:
- The
@ts-expect-errorcomment indicates a type safety issue- No error handling for
getBlockTypes()failures+interface BlockWithEmailSupport extends Block { + supports?: { + email?: boolean; + [key: string]: unknown; + }; +} + export function getAllowedBlockNames(): string[] { - return getBlockTypes() - .filter( ( block: Block ) => { - // @ts-expect-error: 'email' is a custom property - return block.supports?.email === true; - } ) - .map( ( block ) => block.name ); + try { + return getBlockTypes() + .filter( ( block: BlockWithEmailSupport ) => { + return block.supports?.email === true; + } ) + .map( ( block ) => block.name ); + } catch ( error ) { + console.error( 'Failed to get allowed block names:', error ); + return []; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/js/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings(1 hunks)packages/js/email-editor/src/blocks/core/supported-blocks.ts(1 hunks)packages/js/email-editor/src/blocks/index.ts(1 hunks)packages/js/email-editor/src/blocks/utils.ts(1 hunks)packages/js/email-editor/src/editor.tsx(2 hunks)packages/php/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings(1 hunks)packages/php/email-editor/src/Engine/Renderer/readme.md(1 hunks)packages/php/email-editor/src/Engine/class-email-editor.php(2 hunks)packages/php/email-editor/src/Engine/class-settings-controller.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Check Asset Sizes
🔇 Additional comments (10)
packages/php/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings (1)
1-4: LGTM!The changelog entry accurately describes the removal of the static
allowedBlockTypesconfiguration from PHP.packages/js/email-editor/src/editor.tsx (2)
12-12: LGTM!The import statement correctly includes
getAllowedBlockNamesto support the new dynamic block configuration.
36-37: Verify initialization order and consider immutable approach.The logic correctly sets allowed block types dynamically. However, verify that:
setEmailBlockSupport()is called before this component renders to ensure the metadata is available- Consider if mutating the settings object directly could cause issues in React's reconciliation
#!/bin/bash # Description: Verify that setEmailBlockSupport() is called before getAllowedBlockNames() # Expected: setEmailBlockSupport should be called during initBlocks() which happens before editor renders # Check the initialization order in the initialize function rg -A 10 -B 5 "initBlocks.*getAllowedBlockNames" --type ts --type tsxpackages/php/email-editor/src/Engine/Renderer/readme.md (1)
7-7: LGTM!The documentation correctly updates the path to reflect the new location where developers should add email block support.
packages/js/email-editor/src/blocks/index.ts (3)
30-30: LGTM!The import correctly brings in the
setEmailBlockSupportfunction for email block support initialization.
32-32: LGTM!The export correctly makes
getAllowedBlockNamesavailable for use in other modules.
35-35: LGTM!The function call is correctly placed at the beginning of
initBlocks()to ensure email block support is registered before other initialization steps.packages/php/email-editor/src/Engine/class-email-editor.php (1)
290-298: LGTM! Block editor assets loading is correctly implemented.The method properly triggers the
enqueue_block_editor_assetsaction to ensure block types are registered, which is essential for the new JavaScript-based block filtering approach.packages/js/email-editor/src/blocks/core/supported-blocks.ts (1)
10-45: LGTM! Block support filtering is well implemented.The filter approach correctly modifies block registration to add email support metadata. The list of allowed blocks is appropriate for email contexts.
Consider making the block list configurable or documented for easier maintenance:
+/** + * Core blocks that are supported in the email editor. + * This list should be updated when adding or removing email-compatible blocks. + */ const ALLOWED_BLOCK_TYPES = new Set( [packages/php/email-editor/src/Engine/class-settings-controller.php (1)
56-56: ```shell
#!/bin/bashSearch for usage of getAllowedBlockNames and allowedBlockTypes in TS/TSX files
rg -n "getAllowedBlockNames" --glob '.ts' --glob '.tsx' -A5 -B5
rg -n "allowedBlockTypes" --glob '.ts' --glob '.tsx' -A5 -B5</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
lysyjan
force-pushed
the
wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
branch
from
68c01ef to
6b5ab36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/woocommerce/src/Internal/EmailEditor/PageRenderer.php (1)
150-155: Consider the implications of using unstable WordPress API.While the implementation correctly follows WordPress core patterns, note that
wp.blocks.unstable__bootstrapServerSideBlockDefinitions()is marked as unstable. This could potentially break in future WordPress versions.Consider monitoring WordPress core updates for changes to this API or implementing a fallback mechanism. Also, for consistency with line 146, consider using
sprintf()for string formatting:-'wp.blocks.unstable__bootstrapServerSideBlockDefinitions(' . wp_json_encode( get_block_editor_server_block_settings() ) . ');' +sprintf( 'wp.blocks.unstable__bootstrapServerSideBlockDefinitions( %s );', wp_json_encode( get_block_editor_server_block_settings() ) )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/js/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings(1 hunks)packages/js/email-editor/src/blocks/core/supported-blocks.ts(1 hunks)packages/js/email-editor/src/blocks/index.ts(1 hunks)packages/js/email-editor/src/blocks/utils.ts(1 hunks)packages/js/email-editor/src/editor.tsx(2 hunks)packages/php/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings(1 hunks)packages/php/email-editor/src/Engine/Renderer/readme.md(1 hunks)packages/php/email-editor/src/Engine/class-email-editor.php(2 hunks)packages/php/email-editor/src/Engine/class-settings-controller.php(1 hunks)plugins/woocommerce/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings(1 hunks)plugins/woocommerce/src/Internal/EmailEditor/PageRenderer.php(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- plugins/woocommerce/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
- packages/js/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
- packages/php/email-editor/changelog/wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/js/email-editor/src/editor.tsx
- packages/js/email-editor/src/blocks/index.ts
- packages/php/email-editor/src/Engine/Renderer/readme.md
- packages/php/email-editor/src/Engine/class-settings-controller.php
- packages/js/email-editor/src/blocks/core/supported-blocks.ts
- packages/js/email-editor/src/blocks/utils.ts
- packages/php/email-editor/src/Engine/class-email-editor.php
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: JavaScript - @woocommerce/admin-library [unit]
- GitHub Check: PHP: 8.1 WP: latest [WP latest] - @woocommerce/email-editor-php-config [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: Check Asset Sizes
- GitHub Check: build
🔇 Additional comments (1)
plugins/woocommerce/src/Internal/EmailEditor/PageRenderer.php (1)
142-148: LGTM! Good implementation following WordPress core patterns.The addition of block categories loading is well-implemented and follows the WordPress core approach. The comment with the GitHub reference is helpful for understanding the context and rationale.
lysyjan
force-pushed
the
wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
branch
from
6b5ab36 to
d5527e8
Compare
The action enqueue_block_editor_assets loads enqueue scripts containing block from other plugins.
To eliminate warning about missing block categories, we need to add inline script to set categories.
To eliminate warning about missing block titles, we need to add inline script to set categories.
lysyjan
force-pushed
the
wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
branch
2 times, most recently
from
ade49cb to
8fae3cf
Compare
github-actions
bot
added
the
plugin: woocommerce
Testing GuidelinesHi @costasovo , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
costasovo
added
Ballade
costasovo
left a comment
costasovo
left a comment
There was a problem hiding this comment.
@lysyjan Thank you for implementing the API!
I have one suggestion. Please see the other comment. Please note it is not a blocker and we can do it later with server-side render callbacks. I had that idea and wanted to ask for your thoughts.
I tested the editor and I can see all allowed core blocks are present.
| /** | ||
| * Set supports.email = true for the blocks that are supported in the block email editor. | ||
| */ | ||
| export function setEmailBlockSupport() { |
There was a problem hiding this comment.
I have a suggestion here. I think it could be nicer if we moved this to PHP.
The reason is that would have it in one place.
I think we could do that in packages/php/email-editor/src/Integrations/Core/class-initializer.php
We could move the constant with the list of blocks there and add a filter:
add_filter( 'block_type_metadata', array( $this, 'allow_blocks' ) );
/**
* Allow blocks for the email editor.
*
* @param array $block_type_metadata Block type metadata.
*/
public function allow_blocks( array $block_type_metadata ): array {
if ( isset( $block_type_metadata['name'] ) && in_array( $block_type_metadata['name'], $this->allowed_blocks, true ) ) {
if ( ! isset( $block_type_metadata['supports'] ) ) {
$block_type_metadata['supports'] = array();
}
$block_type_metadata['supports']['email'] = true;
}
return $block_type_metadata;
}But when I tried it I ran into an issue that some of the core blocks are registered very early. So we can't register the filter for block_type_metadata in Automattic\WooCommerce\EmailEditor\Initializer::initialize, but we have to do it earlier. It worked for me when I put it directly into Automattic\WooCommerce\EmailEditor\Bootstrap::init.
I think we will need to add rendering callbacks to the core blocks metadata anyway in PHP.
What do you think about moving it to PHP?
There was a problem hiding this comment.
While working on the second ticket, I had a similar idea.
I would merge this pull request and then refactor it.
lysyjan
deleted the
wooplug-4675-load-supported-email-editor-blocks-from-supports-settings
branch
…e#58966) * Add using the action `enqueue_block_editor_assets` to the email editor The action enqueue_block_editor_assets loads enqueue scripts containing block from other plugins. * Set allowed blockTypes dynamically instead of PHP constant * Add a function setting called "supports.email" for supported blocks * Update adding support for a block in readme * Add changelogs * Add loading block categories To eliminate warning about missing block categories, we need to add inline script to set categories. * Add loading server side block definitions To eliminate warning about missing block titles, we need to add inline script to set categories. * Add changelog
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes WOOPLUG-4675
supports.emailproperty.Screenshots or screen recordings:
N/A
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment