Register each reference widget as a separate block #25741
Conversation
adamziel
added
[Feature] Widgets Screen
adamziel
requested review from
youknowriad,
noisysocks,
talldan and
kevin940726
adamziel
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +182 B (0%) Total Size: 1.18 MB
ℹ️ View Unchanged
|
|
Clever! To me, dynamically registering a bunch of blocks doesn't feel any more wrong than dynamically registering a bunch of block variations. The Legacy Widget block is an atypical "block": it's highly coupled to WP Admin and changes depending on what plugins and themes you have activated. What do you think about removing variations altogether and registering one block per widget, regardless of whether it's a callback widget or a class widget? |
| @@ -195,6 +196,7 @@ function gutenberg_get_legacy_widget_settings() { | |||
| 'name' => html_entity_decode( $widget_obj['name'] ), | |||
| 'description' => html_entity_decode( wp_widget_description( $widget_id ) ), | |||
| 'isReferenceWidget' => true, | |||
| 'blockName' => 'core/legacy-widget-' . preg_replace( '#[^a-zA-Z0-9-]#', '-', $widget_id ), | |||
There was a problem hiding this comment.
I don't think we've ever done this but I wonder if we should use sub-namespaces here? In other words, the name would be core/legacy-widget/marquee.
| continue; | ||
| } | ||
| if ( isReferenceWidget ) { | ||
| const blockExists = some( blocksByClientId, { |
There was a problem hiding this comment.
There's probably a nicer way to do this using canInsertBlock or some such.
| ...blockBasis, | ||
| settings: { | ||
| ...blockBasis.settings, | ||
| variations: legacyWidgets |
There was a problem hiding this comment.
Should we go all in with the block per widget approach so that there's only one type of block to test and debug?
| @@ -9,12 +9,22 @@ | |||
| * Register legacy widget block. | |||
| */ | |||
| function register_block_core_legacy_widget() { | |||
| register_block_type_from_metadata( | |||
| $block_type = register_block_type_from_metadata( | |||
There was a problem hiding this comment.
Does using block metadata give us anything at this point? Should we do away with registering the block using metadata altogether and use register_block_type? No need, I think, to pretend that this is a normal block with normal block conventions 🙂
| if ( ! $legacy_widget['isReferenceWidget'] ) { | ||
| continue; | ||
| } | ||
| $legacy_block = clone $block_type; |
There was a problem hiding this comment.
Personally I'd find array_merge easier to understand than cloning and mutating objects.
Doing this would let us implement the sidebar design described in #24870 (comment). |
| ...blockBasis, | ||
| name: widget.blockName, | ||
| metadata: { | ||
| ...blockBasis.metadata, | ||
| name: widget.blockName, | ||
| supports: { | ||
| ...blockBasis.metadata.supports, | ||
| multiple: false, | ||
| }, | ||
| attributes: { | ||
| ...attrs, | ||
| referenceWidgetName: { | ||
| ...attrs.referenceWidgetName, | ||
| default: className, | ||
| }, | ||
| instance: { | ||
| ...attrs.instance, | ||
| default: {}, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Maybe it's time we bring immer into our toolchains 😅
Or before that happens, could we just deepClone the object and mutate it here?
|
I'll abstain from addressing the feedback here until we get consensus about the way forward on #25494. |
aristath
deleted the
update/reference-widgets-as-separate-block-types
branch
Description
Solves #25494
Before this PR, reference widgets were registered as variations of the
legacy-widgetblock. Unfortunately, the requirements for handling reference widgets are very different from regular widgets. One such example is that there should never be more than one reference widget present. This feature is already supported at the block level viasupports.multiple = false. Unfortunately, it's not possible to use that setting for specific variations.This PR attempts to solve the problem by registering each reference widget as a separate block. It is a highly unusual pattern as each block usually comes with it's own block.json and a set of components. That being said, widgets are dynamic in nature, so relying on anything generated during the transpilation wouldn't work.
Let's discuss this approach. Are there any downsides? Are there any better ways of achieving the same result?
Side note: If this PR were to be merged, we should add a lot of inline comments to explain what's happening. As I wasn't sure if it's a viable approach, I didn't include any documentation in this initial attempt.
How has this been tested?
Types of changes
Bug fix
Checklist: