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

Add block-based widget editor #603

Closed
wants to merge 7 commits into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Oct 15, 2020

Trac ticket: https://core.trac.wordpress.org/ticket/51506

Supersedes #588.

This moves the the block-based widget editor over from Gutenberg into Core.

Broadly speaking, there's three parts to this:

  1. @wordpress/edit-widgets is added as a dependency. This is what contains the editor UI.

  2. wp-admin/widgets.php has been modified to branch between the old form-based editor wp-admin/widgets-form.php and the new block-based editor wp-admin/widgets-block-editor.php depending on get_theme_support( 'widgets-block-editor' ) and use_widgets_block_editor.

  3. Supporting infrastructure such as WP_Widget_Block and widgets.php?widget-preview={} has been copied over from Gutenberg.

This PR contains WP_REST_Sidebars_Controller and WP_REST_Widget_Utils_Controller so that it's easier to test, but these parts should be committed separately.

How to test

  1. npm run env:start
  2. npm run env:install
  3. Browse to Appearance → Widgets.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

echo do_blocks( $instance['content'] );
$textarea_id = $this->get_field_id( 'content' );
?>
<br/>
Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel: Do you know if the <br /> is necessary? It doesn't seem to do much.

$textarea_id = $this->get_field_id( 'content' );
?>
<br/>
<textarea id="<?php echo $textarea_id; ?>" name="<?php echo $this->get_field_name( 'content' ); ?>"
Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel: Do you know why this is a hidden textarea and not an <input type="hidden">?

*/
public function form( $instance ) {
$instance = wp_parse_args( (array) $instance, $this->default_instance );
echo do_blocks( $instance['content'] );
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't port over the hack that Gutenberg does to ensure that <form> elements don't get nested within other <form> elements here because I'd like to find a better way of fixing that.

@@ -262,6 +262,8 @@
require ABSPATH . WPINC . '/rest-api/endpoints/class-wp-rest-block-directory-controller.php';
require ABSPATH . WPINC . '/rest-api/endpoints/class-wp-rest-application-passwords-controller.php';
require ABSPATH . WPINC . '/rest-api/endpoints/class-wp-rest-site-health-controller.php';
require ABSPATH . WPINC . '/rest-api/endpoints/class-wp-rest-sidebars-controller.php';
require ABSPATH . WPINC . '/rest-api/endpoints/class-wp-rest-widget-utils-controller.php';
Copy link
Member Author

Choose a reason for hiding this comment

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

These files are included in this PR for testing purposes. They'll land separately in https://core.trac.wordpress.org/ticket/51460.

@noisysocks
Copy link
Member Author

OK, I think this is ready to be reviewed and committed. The REST API endpoints are included so that it's easier to test, but these should be added separately via https://core.trac.wordpress.org/ticket/51460 before this PR is committed.

@talldan
Copy link
Contributor

talldan commented Oct 19, 2020

@noisysocks Just noticed we'll also have to make sure $current_screen->is_block_editor() is set to true for the widget screen as per WordPress/gutenberg#26263

@noisysocks
Copy link
Member Author

@noisysocks Just noticed we'll also have to make sure $current_screen->is_block_editor() is set to true for the widget screen as per WordPress/gutenberg#26263

I'm a little unclear on whether $current_screen->is_block_editor() should mean this page is the post block editor or this page has a block editor on it. I'll set it for now and let's see how that goes.

@noisysocks noisysocks closed this Oct 26, 2020
@adamziel
Copy link
Contributor

adamziel commented Oct 26, 2020

@noisysocks it means this page is a full-screen block editor - potentially specific to post/page. It does not mean this page has any block editor somewhere on it. the entire reason for this change: https://core.trac.wordpress.org/ticket/51330

@noisysocks
Copy link
Member Author

@noisysocks it means this page is a full-screen block editor - potentially specific to post/page. It does not mean this page has any block editor somewhere on it. the entire reason for this change: https://core.trac.wordpress.org/ticket/51330

Got it. We'll need to take a different approach in WordPress/gutenberg#26263, then. cc. @talldan

@talldan
Copy link
Contributor

talldan commented Nov 2, 2020

@noisysocks @adamziel What does 'full-screen' mean in this context? The class seems to have been added in WordPress 5.0 quite a long time before the post editor was full screen by default (https://core.trac.wordpress.org/changeset/44133, https://core.trac.wordpress.org/ticket/45037), so it seems like it should be perfectly fine to have a non-fullscreen block editor but still have is_block_editor() return true. Core also doesn't have any styles related to is-fullscreen-mode, it's all handled by the interface package.

I just want to question the assumptions here since I don't see why the widgets screen would have is_block_editor() return false when it is a block editor.

I think what we can all agree on is that it definitely seems like this one boolean has way too much responsibility, and it should probably be made more granular along the lines of https://core.trac.wordpress.org/ticket/51330. Full screen mosde should also not be implicit to is_block_editor() == true IMO.

However, as part of that, I think we should try to make is_block_editor() return the right thing, or deprecate it entirely.

@adamziel
Copy link
Contributor

adamziel commented Nov 2, 2020

What does 'full-screen' mean in this context?

The is-fullscreen CSS class, not sure if anything else. I remember setting is_block_editor true caused JS errors in the widgets editor for me a few months back, but I am unable to reproduce this behavior now so maybe we're good. I went through all the usages of is_block_editor() and it doesn't seem like there is much more to it...?

I just want to question the assumptions here since I don't see why the widgets screen would have is_block_editor() return false when it is a block editor.

@talldan you hit the nail on the head. I'm all for marking the widgets editor as a block_editor as you said. Saying it's not an editor when it is one is counter-intuitive. Maybe the implicit fullscreen mode is the only blocker for that - how about another boolean parameter to control just this aspect of displaying? e.g. is_full_screen_editor() that defaults to true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants