#56408 closed task (blessed) (fixed)
Blocks: Allow registering multiple items for all supported asset types
Reported by: | gziolo | Owned by: | gziolo |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch has-unit-tests needs-dev-note |
Focuses: | Cc: |
Description
Part of https://github.com/WordPress/gutenberg/issues/41236. More details in https://github.com/WordPress/gutenberg/issues/33542.
This PR is heavily inspired by the work done by @zieladam in #56094.
The idea is to allow more than one script per block for editorScript
, script
, and viewScript
. @aristath already added that capability for style
and editorStyle
a long time ago with https://github.com/WordPress/gutenberg/pull/32510. It was backported to WordPress core using the same hooks that Gutenberg uses with [52069] as part of #54337, which causes issues like the one reported in https://github.com/WordPress/gutenberg/issues/43086. Here, the code gets refactored for style
, editorStyle
so multiple assets are supported natively in WP_Block_Type
class and in the REST API endpoint for block types. The same implementation is mirrored for scripts: editorScript
, script
and viewScript
.
Attachments (1)
Change History (31)
This ticket was mentioned in PR #3108 on WordPress/wordpress-develop by gziolo.
2 years ago
#1
2 years ago
#2
Brilliant! Thank you so much for working on this @gziolo 👍
I haven't tested the implementation yet, but the code looks good and makes perfect sense 🎉
2 years ago
#3
@gziolo The following block.json:
"style": ["wp-block-button", "file:test.css", "wp-column-block"]
Yields the following HTML:
{{{html
<link rel='stylesheet' id='wp-block-button-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-2-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-3-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
}}}
So something isn't right yet.
2 years ago
#4
This code in register_block_style_handle
overrides the original style path:
// Check whether styles should have a ".min" suffix or not. $suffix = SCRIPT_DEBUG ? '' : '.min'; $style_uri = plugins_url( $style_path, $metadata['file'] ); if ( $is_core_block ) { $style_path = "style$suffix.css"; $style_uri = includes_url( 'blocks/' . str_replace( 'core/', '', $metadata['name'] ) . "/style$suffix.css" ); }
2 years ago
#5
This code in register_block_style_handle overrides the original style path:
@adamziel, great catch. It's tricky to fix as it's correct today for existing core blocks only because it's always the style targeting the block, and subsequent styles are handled with the hook. If someone would change the order, then a similar issue would apply. It seems like we need to redo the special handling for styles in core blocks.
2 years ago
#6
The case prefixed with
file:
would be pretty simple to exclude from this special handling. I guess that style from a different block"wp-column-block"
could bail out early as it should get registered elsewhere.
It could make sense for blocks that depend on other blocks or block-like elements, e.g. the search block may want to include the button block's styles.
2 years ago
#7
@gziolo The following block.json:
"style": ["wp-block-button", "file:test.css", "wp-column-block"]Yields the following HTML:
<link rel='stylesheet' id='wp-block-button-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' /> <link rel='stylesheet' id='wp-block-button-2-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' /> <link rel='stylesheet' id='wp-block-button-3-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
The way I approached core blocks, it works as follows:
- you can only pass style handles like it was possible before, entries starting with
file:
will get ingored - only the first style handle passed gets special treatment
- additional style handles needs to be registered elsewhere
2 years ago
#8
@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.
2 years ago
#9
@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.
Thank you, @gziolo! It looks like we will eventually need it to unblock the sync. We have another week until Feature Freeze -- do you think that's enough time to get feedback and potentially make changes? If it isn't, it might be better to merge it as-is and iterate on feedback in a separate issue/PR 😊
#11
@
2 years ago
- Owner set to gziolo
- Resolution set to fixed
- Status changed from assigned to closed
In 54155:
2 years ago
#13
@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.
Thank you, @gziolo! It looks like we will eventually need it to unblock the sync. We have another week until Feature Freeze -- do you think that's enough time to get feedback and potentially make changes? If it isn't, it might be better to merge it as-is and iterate on feedback in a separate issue/PR 😊
I committed all the changes with https://core.trac.wordpress.org/changeset/54155 to unblock the Gutenberg sync process. I'll watch this PR and the Trac ticket for further feedback and take the necessary actions.
#14
@
2 years ago
@gziolo:
In the backwards compat layer, there are a few $this->{$new_name }[0]
with an extra space after new_name
that could/should be removed.
#17
@
2 years ago
It looks like [54155] broke loading of style.css
files, namely it's enqueuing style.css
files that don't exist on the frontend, which leads to 404 errors.
I'm for example getting the ones from the attached screenshot. All these style.css
files don't exist in Core.
The errors don't appear when working on a checkout of the commit's predecessor, r54154.
#20
@
2 years ago
@TobiasBg, thank you for the report. As far as I can tell, all the blocks that trigger 404 don't have style
field in block.json
so it must be one of the filters that inject this type of style to all core blocks with no content. I will investigate.
This ticket was mentioned in PR #3328 on WordPress/wordpress-develop by gziolo.
2 years ago
#21
Trac ticket: https://core.trac.wordpress.org/ticket/56408
#22
@
2 years ago
I included the patch on GitHub that hopefully will fix the issue with 404 HTTP codes for blocks that don't have style files but still register handles.
2 years ago
#24
Committed with https://core.trac.wordpress.org/changeset/54323.
This ticket was mentioned in PR #3395 on WordPress/wordpress-develop by aristath.
2 years ago
#25
It appears that an extra condition was preventing multiple scripts from loading in blocks like the navigation block - where we need a separate script for the modal.
This patch removes an additional condition that is not necessary.
Whether a style or script gets enqueued should not be related to the block's render callback.
Trac ticket: https://core.trac.wordpress.org/ticket/56408
c4rl0sbr4v0 commented on PR #3395:
2 years ago
#26
Nice job!! Work as expected
2 years ago
#27
Note: This was reported in https://github.com/WordPress/twentytwentythree/issues/214, then https://github.com/WordPress/gutenberg/issues/44638
dream-encode commented on PR #3395:
2 years ago
#29
Merged into core in https://core.trac.wordpress.org/changeset/54367.
#30
@
2 years ago
[54367] effectively closes #56470 as resolved. It will have to be included in the dev note for Block API changes in WordPress 6.1. We should also update the following section in the documentation:
In particular, the following section must be updated to reflect the changes applied:
viewScript (when the block defines render_callback during registration in PHP or a render field in its block.json, then the script is registered but the block author is responsible for enqueuing it)
I agree that the old behavior was too confusing, and we seek alternative ways to allow conditional enqueueing of view scritps.
Trac ticket: https://core.trac.wordpress.org/ticket/56408
Part of https://github.com/WordPress/gutenberg/issues/41236. More details in https://github.com/WordPress/gutenberg/issues/33542.
This PR is heavily inspired by the work done by @adamziel in https://github.com/WordPress/wordpress-develop/pull/2853.
The idea is to allow more than one script per block for
editorScript
,script
, andviewScript
. @aristath already added that capability forstyle
andeditorStyle
a long time ago with https://github.com/WordPress/gutenberg/pull/32510. It was backported to WordPress core using the same hooks that Gutenberg uses, which causes issues like the one reported in https://github.com/WordPress/gutenberg/issues/43086. Here, the code gets refactored forstyle
,editorStyle
so multiple assets are supported natively inWP_Block_Type
class and in the REST API endpoint for block types. The same implementation is mirrored for scripts:editorScript
,script
andviewScript
.