First stab at sidebars_widgets-based widget management#24290
Conversation
adamziel
added
[Block] Legacy Widget
|
Size Change: +1.89 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
Looks promising. It's amazing just how similar this ends up being to how the Navigation screen works. wp-rest-api-sidebars is a nice find! I think let's copy |
adamziel
requested review from
Soean,
TimothyBJacobs,
ajitbohra,
nerrad,
spacedmonkey,
talldan and
youknowriad
as code owners
| 'name' => 'Text', | ||
| 'description' => 'Arbitrary text.', | ||
| 'number' => 1, | ||
| 'rendered' => ' <div class="textwidget">Custom text test</div>' . "\n ", |
There was a problem hiding this comment.
What causes all the empty space? I wonder if it makes sense to trim the rendered form?
TimothyBJacobs
left a comment
TimothyBJacobs
left a comment
There was a problem hiding this comment.
Thanks for adding the PHP tests they're looking good! Let's also add coverage for when the user is logged out or has insufficient permissions.
It'd probably also be good to have a test that covers the slashing behavior specifically. Those can get tricky.
|
Any thoughts on having a new Gutenberg Block widget vs. using the existing HTML widget? |
There was a problem hiding this comment.
Hi @adamziel thank you for continuing the widgets work, and for all the work on this PR 👍
Generally, it seems the PR in on a good path, I left some initial comments.
I'm having some problems when testing the branch:
I get the following error:
Target container is not a DOM element.
at http://yah.local/wp-includes/js/dist/vendor/react-dom.js?ver=16.9.0:25173:28
at render (http://yah.local/wp-includes/js/dist/vendor/react-dom.js?ver=16.9.0:25176:7)
at Module.customizerInitialize (http://yah.local/wp-content/plugins/gutenberg/build/edit-widgets/index.js?ver=2ebbe5d16afdab3466a61f4165697cd3:1914:68)
I got this error even when loading the old screen. But on master, I don't have these errors it seems we are now running customizer specific code on both the old and new widgets screen?
I was able to test with success (excluding the errors referred above) the interoperability between the old and new screens I was able to reorder blocks on the old screen :) Nice work accomplishing this!
| method="post" | ||
| onBlur={ () => { | ||
| if ( this.shouldTriggerInstanceUpdate() ) { | ||
| if ( isReferenceWidget ) { |
There was a problem hiding this comment.
The non-class based widgets render their own form and perform their own save sometimes using tables they created. This data is outside the structure we use to control the widgets and I guess we need a specific save for it, it can be automatically or we can have a save button on the legacy widgets block.
| const [ blocks, onInput, onChange ] = useEntityBlockEditor( | ||
| KIND, | ||
| POST_TYPE, | ||
| { id: buildWidgetAreasPostId() } |
There was a problem hiding this comment.
What is the reason that makes us need to have a post id and use useEntityBlockEditor here? Using it inside the widget area block causes some problem?
| @@ -137,6 +137,7 @@ function gutenberg_get_legacy_widget_settings() { | |||
| foreach ( $wp_widget_factory->widgets as $class => $widget_obj ) { | |||
| $available_legacy_widgets[ $class ] = array( | |||
There was a problem hiding this comment.
Right now I'm seeing "Gutenberg block" as an available legacy widget I guess we need to filter it ?
| function gutenberg_widgets_init( $hook ) { | ||
| if ( 'gutenberg_page_gutenberg-widgets' !== $hook && 'gutenberg_customizer' !== $hook ) { | ||
| return; | ||
| if ( ! in_array( $hook, array( 'gutenberg_page_gutenberg-widgets', 'gutenberg_customizer', 'widgets.php' ) ) ) { |
There was a problem hiding this comment.
We don't need to run all of this function in widgets.php, just wp_enqueue_style( 'wp-block-library' );
| * @param array $instance Settings for the current Block widget instance. | ||
| */ | ||
| public function widget( $args, $instance ) { | ||
| echo do_blocks( $instance['content'] ); |
There was a problem hiding this comment.
This will sometimes render a <form> (e.g. Search block), which is invalid HTML, because we'll then have a <form> nested within a <form>.
There was a problem hiding this comment.
@noisysocks I don't see a <form> anywhere in this file:
https://github.com/WordPress/gutenberg/tree/master/packages/block-library/src/search/edit.js
Where I can see a <form> is in the legacy-widget block:
I don't think that's a problem though - the widget screen is not wrapped in a <form> and so each block is free to render anything it wants. I'm going to mark the related to-do item as complete, feel free to mark it as incomplete if I'm wrong here.
There was a problem hiding this comment.
Sorry, I wasn't clear here. I'm referring to the legacy widgets.php screen. By default every widget is wrapped with a <form>.
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/widgets.php#L205
This means that you can sometimes end up with invalid HTML.
- Navigate to Widgets (beta) and add a Search block to one of the block areas.
- Navigatie to Widgets (the old screen).
- View the page source and search for
wp-block-search.
You'll see that there's a <form> inside a <form>. Firefox helpfully highlights the invalid HTML.
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
I think the direction here is solid and that it's the right way to go. There's lots of remaining work, but we will be here forever addressing it in this already very large PR. Since this is all behind a feature flag, let's merge it now and address the feedback in more manageable follow-up PRs.
Thanks @adamziel! Nice work. I think we can get this feature in for WP 5.6 💪
|
I created #24530 which summarises and tracks the necessary follow-up work here. |
5 participants
Description
This is an attempt at making the experimental widget management screen fully functional so that it's possible to mix&match legacy widgets and gutenberg blocks.
It works as follows:
WP_Widget_Blockclass.Other considerations:
/widget-utilscontroller is still there and would be a good candidate for removal in some future iteration.Reuses parts of the following plugin (CC @martin-pettersson) https://github.com/martin-pettersson/wp-rest-api-sidebars
How has this been tested?
Screenshots
Types of changes
Checklist: