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

Customizer: Add widget blocks section. #16204

Merged
merged 16 commits into from Jul 4, 2019
Merged

Conversation

@epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Jun 17, 2019

cc @aduth @jorgefilipecosta

Closes #13205

Description

This PR adds the widget blocks editor to a new section in the Customizer. The wp-edit-widgets package that powers the new widgets page in WPAdmin has been used for this.

This is still a work in progress as wp-edit-widgets appears to have compatibility issues with legacy widgets. Some combinations of legacy widgets and widget blocks in some themes, e.g. Twenty Nineteen, break the Customizer or sometimes don't save the widgets for the right widget content area. All of the issues seem to be related to how wp-edit-widgets persists data.

After those issues are resolved, more work needs to be done to make the live preview work without page refreshes.

Updates

How has this been tested?

This has only been tested manually.

Screenshots

Customizer Widget Blocks Editor

Types of Changes

New Feature: Add a widget blocks section to the Customizer.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 17, 2019

The issues have been fixed. They were caused by an incompatibility between how widget blocks are stored and legacy widgets sanitization for the Customizer. Next up, live-preview enhancements!

Relevant commit:

4899f9a Customizer: Fix incompatibility with legacy widgets sanitization.

Loading

.gitignore Outdated
@@ -12,6 +12,7 @@ phpcs.xml
yarn.lock
docker-compose.override.yml
/wordpress
.vscode
Copy link
Member

@gziolo gziolo Jun 18, 2019

Choose a reason for hiding this comment

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

You can create a global gitignore file instead to so you don't have to include IDE specific ignores to each project:
https://help.github.com/en/articles/ignoring-files#create-a-global-gitignore

We would have to keep a long list of IDE specific files for each WordPress project instead :)

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Done, thanks 👍

Loading

* Start: Include for phase 2
*
* @package gutenberg
* @since x.x.x
Copy link
Member

@gziolo gziolo Jun 18, 2019

Choose a reason for hiding this comment

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

Next planned Gutenberg release is 6.0.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

👍

Loading

@@ -58,15 +58,21 @@ function gutenberg_widgets_init( $hook ) {
wp_add_inline_script(
'wp-edit-widgets',
sprintf(
'wp.editWidgets.initialize( "widgets-editor", %s );',
'window.addEventListener( "load", function() {
wp.editWidgets.initialize( "widgets-editor", %s );
Copy link
Contributor

@youknowriad youknowriad Jun 18, 2019

Choose a reason for hiding this comment

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

I think the theory here is that editWidgets is specific to the widgets screen and we didn't initially think of using it in the customizer directly.

I can see how it's a quick way to arrive to something usable, the issue though is that it forces us to play with CSS to have a sidebar with the "save" button...

I think ideally, we'd:

  • Extract the common parts between the customizer and the widgets screen into a @wordpress/widgets or @wordpress/widget-areas package
  • Build a new widgets-customizer-panel package that use the common parts in order to bootstrap the customizer package.
  • Try to merges the save button with the customizer's save button. (We shouldn't need two separate saving actions)

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

What if we do that all in the same, current package. Merging the save buttons when rendering in the customizer should be simpler than splitting this into multiple packages.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Hi @epiqueras, thank you for working on this task! There is good progress here in a short amount of time :)
I guess having a separate package for the customizer is going to be beneficial down the road as @youknowriad suggested.
I think we should also have a customizer.php where we add the customizer specific PHP code.

Loading

'wp.blocks.unstable__bootstrapServerSideBlockDefinitions(' . wp_json_encode( get_block_editor_server_block_settings() ) . ');'
);

if ( 'gutenberg_customizer' !== $hook ) { // `get_block_editor_server_block_settings` is undefined in the Customizer.
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jun 18, 2019

Choose a reason for hiding this comment

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

It will be a complex problem to solve; we can implement a core patch to make the function available in the customizer. But users of the plugin using the current WP version would notice some crashes, maybe there is some init function we can call, but I guess for now we can add a "Todo:" so we don't miss the need to fix this.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Adding the comment for now.

Loading

Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

What does this mean exactly? Block registered server side won't be available in the customizer?

Loading

Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

Can't we just "include" the file where it's defined?

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

What does this mean exactly? Block registered server side won't be available in the customizer?

Yes

Can't we just "include" the file where it's defined?

Huh, I got rid of the if guard and it worked. I don't see any obvious changes in Core that would have fixed this 🤔

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

Loading

gutenberg.php Outdated
* @param \WP_Customize_Manager $wp_customize An instance of the class that controls most of the Theme Customization API for WordPress 3.4 and newer.
* @since x.x.x
*/
function gutenberg_customize_register( $wp_customize ) {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jun 18, 2019

Choose a reason for hiding this comment

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

This code is experimental, and if we need to merge the plugin to the core while we are not finished yet having the code in the middle of the file may make the task harder and more error-prone. I guess we can follow a similar approach to what we did with the widgets screen and have a separate file called customizer.php that contains this code and then we can require this code in load.php.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Good idea.

Loading

@@ -58,15 +58,21 @@ function gutenberg_widgets_init( $hook ) {
wp_add_inline_script(
'wp-edit-widgets',
sprintf(
'wp.editWidgets.initialize( "widgets-editor", %s );',
'window.addEventListener( "load", function() {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jun 18, 2019

Choose a reason for hiding this comment

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

The core version to load the block editor uses:

wp.domReady( function() {
	resolve( wp.editPost.initializeEditor( 'editor', "%s", %d, %s, %s ) );
} );

I guess we can use the same function here.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Awesome, didn't know we had this!

Loading

lib/widgets.php Outdated
@@ -213,3 +213,23 @@ function gutenberg_create_wp_area_post_type() {
add_action( 'init', 'gutenberg_create_wp_area_post_type' );

add_filter( 'sidebars_widgets', 'Experimental_WP_Widget_Blocks_Manager::swap_out_sidebars_blocks_for_block_widgets' );

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jun 18, 2019

Choose a reason for hiding this comment

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

I guess we can move this logic to a customizer.php file.
For reference, this code was added because of the global $wp_registered_widgets used in this function:

	public function sanitize_sidebar_widgets_js_instance( $widget_ids ) {
		global $wp_registered_widgets;
		$widget_ids = array_values( array_intersect( $widget_ids, array_keys( $wp_registered_widgets ) ) );
		return $widget_ids;
	}

Does not contains the old format (an array of widgets) but contains the new format (a post id), I guess the best solution is to undestand why the global does not contain the backcompatible format and fix that problem as it may cause problems with plugins. If that is not possible I guess we can use this filter but instead of unsetting sanitize_js_callback I think we can make it point to new function where we compute the correct inputs based on the post id and we pass this values to the function that was set in $args['sanitize_js_callback'] before.
This does not seem a problem form this PR so I think we can address it separately provided we have a "Todo:" note to not miss this task.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jun 18, 2019

Choose a reason for hiding this comment

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

Yeah I wrote that in the documentation:

gutenberg/lib/widgets.php

Lines 218 to 221 in 4899f9a

* Filters the Customizer widget settings arguments.
* This is needed because the Customizer registers settings for the raw registered widgets, without going through the `sidebars_widgets` filter.
* The `WP_Customize_Widgets` class expects sidebars to have an array of widgets registered, not a post ID.
* This results in the value passed to `sanitize_js_callback` being `null` and throwing an error.

But, the issue is that $widget_ids is an int when it should be an array.

This filter,

public static function swap_out_sidebars_blocks_for_block_widgets( $sidebars_widgets_input ) {
, fixes that, but it doesn't run for that part of core.

I've moved it to customizer.php and added a TODO.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 18, 2019

@gziolo 's feedback.

Relevant commit:

65360e1 Customizer: Set @since versions for new PHP methods.

Also remove the .vscode entry in .gitignore.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 18, 2019

@jorgefilipecosta 's feedback.

Relevant commit:

7fa5a38 Customizer: Refactor new PHP code so that it can be loaded conditionally.

Also add missing TODOs.

The next tasks would be not rendering the Update button when on the Customizer and integrating into the Customizer's save and live preview functionality.

Loading

@epiqueras epiqueras self-assigned this Jun 20, 2019
@epiqueras epiqueras added this to the Future milestone Jun 20, 2019
@epiqueras epiqueras force-pushed the add/widget-blocks-to-the-customizer branch from 7fa5a38 to 271a49f Jun 25, 2019
@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 25, 2019

Saving customizer changes now also saves widget block edits and live preview is working:

271a49f Customizer: Integrate widget blocks live preview and saving.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 25, 2019

Kapture 2019-06-25 at 13 18 54

Loading

Copy link
Contributor

@youknowriad youknowriad left a comment

I took this for a spin, and it's great. I feel with the saving and previewing we kind of addressed the most difficult challenges of this screen.

Some things that would be good to improve.

  • I think the "Inspector" should probably be shown in a modal instead of a sidebar in that screen. This is doable by adding a button to toggle the modal to the Block settings menu (there's a lot for that). By I feel it could be left for a separate PR and just remove the inspector from the initial implementation.

  • Instead of reusing wp.editWidgets.initialize( "widgets-editor", %s ); and trying to detect whether we're the customizer or widgets screen using DOM or availability of globals, I think we should have:

    • a separate wp.editCustomizerWidgetsPanel function to render a separate React Tree that is directly optimized for the customizer screen.
    • reuse as much as we can from the edit-widgets Tree of components (WidgetArea...)
    • Avoid rendering the Inspector in that customizer tree (we could use a modal but could be a separate PR)
  • I think there are some z-index issues for the block toolbar and styling issues for the areas (the width is too small by default) but these tweaks can also be addressed separately.

What do you think?

Loading

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 25, 2019

I also had some PHP notices while saving the areas

Notice: Undefined index: blocks-widget-4ad2ac37d121d5a87a8176a54caa2e5e in /var/www/html/wp-includes/class-wp-customize-widgets.php on line 497 
Notice: Undefined index: blocks-widget-4ad2ac37d121d5a87a8176a54caa2e5e in /var/www/html/wp-includes/class-wp-customize-widgets.php on line 509 
Notice: Undefined index: blocks-widget-4ad2ac37d121d5a87a8176a54caa2e5e in /var/www/html/wp-includes/class-wp-customize-widgets.php on line 510 
Notice: Undefined index: blocks-widget-4ad2ac37d121d5a87a8176a54caa2e5e in /var/www/html/wp-includes/class-wp-customize-widgets.php on line 579 
Notice: Undefined index: blocks-widget-ccd122757280cfba6bd1804a475deccc in

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jun 25, 2019

@youknowriad

I think the "Inspector" should probably be shown in a modal...

Yeah, that would look a lot better. I'll follow up this PR with that as suggested.

Instead of reusing wp.editWidgets.initialize...

This makes a lot of sense, specially because the current tree is still being modified in parallel, but what if we have wp.editWidgets.initializeWidgetsScreen and wp.editWidgets.initializeWidgetsCustomizerSection. It seems like that would make it simpler to share the code.

I think there are some z-index issues for the block toolbar...

I'll fix those in the new component tree in this PR, should be simple.

I also had some PHP notices while saving the areas...

Looks like new Customizer PHP incompatibilities with post IDs being used as sidebar content. There's probably a hook to bailout from whatever it's trying to do, I'll look into it after the other changes.

Loading

// Subscribe to registry changes so that we can update the preview
// and the hidden input's value when any of the widget areas change.
widgetAreas = getWidgetAreasObject();
window.wp.data.subscribe( () => {
Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

Are we considered in terms of performance here? I feel we don't really need to support the live-preview, especially because the rendered HTML can be a lot different from the HTML used in the blocks. We can have dynamic blocks, we can have server-side hooks... I feel it's just going to be confusing.

That said, I don't think it's a blocker for this PR.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

Are we considered in terms of performance here?...

I think that with the throttling in place, the performance hit is minimal, although I haven't done any serious testing besides 6x CPU slowdown on my machine working fine.

...especially because the rendered HTML can be a lot different from the HTML used in the blocks...

I have plans for another PR that adds an asynchronous serialize that uses <ServerSideRender /> to render dynamic blocks. I could also look at time-slicing or offloading serialization to another thread and that would also take care of any performance concerns here.

Loading

* @yields {Object} Action object.
*/
export function* setupWidgetAreas() {
export function* setupWidgetAreas( savedWidgetAreas ) {
Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

I'm not sure why do we need this change here. Can't we just update the blocks in the store once we edit them? and rely on the fact that the store is always up to date?

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

It's always up to date with the "published" widgets.

This allows us to override specific widget areas with their Changeset counterparts.

Loading

Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

I'm not certain I grasp the details yet but I think the edit-widgets store is used to track the current state of the editor, so maybe we should track the changeset couterpart instead of the published widgets. cc @jorgefilipecosta

What value do we have in tracking the published widgets there?

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

...I think the edit-widgets store is used to track the current state of the editor...

Yes

setupWidgetAreas loads the published widget areas into the store when the app starts. What I did here was provide a way for the Customizer sync code to replace any combination of those widget areas with what is saved in the Changeset. The published widget areas are mapped into the objects used by the store, but their blocks property is set to the Changeset value, if defined:

blocks: ( savedWidgetAreas && savedWidgetAreas[ id ] ) || parse( get( content, [ 'raw' ], '' ) ),

Loading

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

It seems we are calling setupWidgetAreas just as a way to replace the blocks with the customizer changeset version. Can't we just do a loop call to updateBlocksInWidgetArea to replace blocks with customizer changeset version, avoiding the need to change this action?

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

Loading

if ( window.wp && window.wp.customize && window.wp.data ) {
// Wait for the Customizer to finish bootstrapping.
window.wp.customize.bind( 'ready', () =>
window.wp.customize.previewer.bind( 'ready', () => {
Copy link
Contributor

@youknowriad youknowriad Jul 2, 2019

Choose a reason for hiding this comment

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

I do wonder if we can attach these behaviors to an effect in the Initializer component for instance, I think it makes it a bit easier to reason about to think about.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

That was the original approach, but we pivoted a little bit here:

#16204 (comment)

Loading

Copy link
Contributor

@youknowriad youknowriad left a comment

This seems almost ready for a v1.

I'd appreciate a second review @jorgefilipecosta @aduth and others.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jul 2, 2019

b3211fb Customizer: Add server registered blocks back in. The getter function is now defined?

Loading

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Hi @epiqueras, excellent work here, it is fantastic that you managed to find solutions that allow us to test complex feature like the customizer previews 👍
I think this is almost ready to merge; I left some comments that I think we should consider before the merge.
They are mostly changes to reduce the impact on users that are not trying the block widgets experiments.

Loading

wp_register_widget_control(
$widget_id,
__( 'Blocks Area', 'gutenberg' ),
'noop',
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

I'm getting the following warning:
call_user_func_array() expects parameter 1 to be a valid callback, function 'noop' not found or invalid function name

I guess in PHP we don't have a defined noop callback.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

I wonder why I didn't get that error.

I'll change it to 'echo'.

Loading

// Check that all the necessary globals are present.
if ( window.wp && window.wp.customize && window.wp.data ) {
// Wait for the Customizer to finish bootstrapping.
window.wp.customize.bind( 'ready', () =>
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

Would it make sense to just perform this initialization when the user opens the "Block Widgets" section?
Right now for users that never used our experimental block widget features and have normal widgets we are breaking the ability for them to preview the widgets. previewBlocksInWidgetArea ends being called with a set of legacy widget blocks (just HTML comments) this changes the dom the Widgets section was expecting and makes it impossible to see the previews.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

I am changing it to run after the editor finishes setup so that it only fires after the first edit.

Loading

`[data-customize-partial-placement-context*='"sidebar_id":"${ id }"']`
);
if ( widgetArea ) {
widgetArea.innerHTML = serialize( blocks );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

Related to another comment, maybe if all the blocks are legacy widget blocks, we should not update the preview. Users that just have normal widgets and never used our new block mechanism may open the Block Widgets section, and when they do it, they will lose the widget previews right away even if they go back to the standard widget sections, this check will reduce a little bit that problem.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

#16204 (comment)

That fixes this too, but widget block changes will still take precedence over legacy widget ones. I think we should just show one or the other, having both is confusing.

Loading

);
$wp_customize->add_section(
'gutenberg_widget_blocks',
array( 'title' => __( 'Widget Blocks', 'gutenberg' ) )
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

Would it make sense to name this section 'Widget Blocks (Experimental)', to make users more aware of the experimental nature of this section? Similar to what we did with the widget blocks screen and the legacy widgets block.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

Yes

Loading

* @yields {Object} Action object.
*/
export function* setupWidgetAreas() {
export function* setupWidgetAreas( savedWidgetAreas ) {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

It seems we are calling setupWidgetAreas just as a way to replace the blocks with the customizer changeset version. Can't we just do a loop call to updateBlocksInWidgetArea to replace blocks with customizer changeset version, avoiding the need to change this action?

Loading

if ( null === self::$unfiltered_sidebar_widgets ) {
self::$unfiltered_sidebar_widgets = $sidebars_widgets;
}
$changeset_data = null;
if ( is_customize_preview() ) {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 2, 2019

Choose a reason for hiding this comment

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

Maybe a function exists check on is_customize_preview provides extra security to not break special sites that may not have this function defined.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

It seems we are calling setupWidgetAreas just as a way to replace the blocks with the customizer changeset version. Can't we just do a loop call to updateBlocksInWidgetArea to replace blocks with customizer changeset version, avoiding the need to change this action?

Good catch! This is actually not needed anymore after the changes for supporting share URLs. Changeset data is already rendered on the server.

Maybe a function exists check on is_customize_preview provides extra security to not break special sites that may not have this function defined.

Yes!

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 2, 2019

Choose a reason for hiding this comment

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

Changeset data is already rendered on the server.

Actually, no, because the store loads from an API that doesn't go through that filter, I'll go with the updateBlocksInWidgetArea approach after setup and before the subscription.

Loading

- Guard for undefined functions.
- Add "experimental" warning to section title.
- Don't let customizer syncing overwrite legacy widget previews until after the first edit.
@epiqueras epiqueras force-pushed the add/widget-blocks-to-the-customizer branch from 649b375 to a86e801 Jul 2, 2019
@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jul 2, 2019

@jorgefilipecosta 's feedback

a86e801 Customizer: Fix legacy compatibility issues.

  • Guard for undefined functions.
  • Add "experimental" warning to section title.
  • Don't let customizer syncing overwrite legacy widget previews until after the first edit.

Loading

wp_register_widget_control(
$widget_id,
__( 'Blocks Area', 'gutenberg' ),
'echo',
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 3, 2019

Choose a reason for hiding this comment

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

This still triggers an error:

call_user_func_array() expects parameter 1 to be a valid callback, function 'echo' not found or invalid function name

It seems echo is not considered a callback function, but using a function that prints something would be dangerous anyway as printing something during REST request may make the requests invalid.
It seems our best bet is to create a simple noop function ourselves and pass its name here.

Loading

Copy link
Contributor Author

@epiqueras epiqueras Jul 3, 2019

Choose a reason for hiding this comment

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

Will do that 👍

Loading

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

I'm noticing a bug that was not happening in the tests yesterday, not sure if it is something in my setup, it would be good to have a double check here:
If we use the normal widget screen to set up widgets and then we go to the customizer we delete the legacy widget blocks, we add some blocks (paragraphs) and we press publish right away without saving a draft first, the saving does not happen.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jul 3, 2019

I'm noticing a bug that was not happening in the tests yesterday, not sure if it is something in my setup, it would be good to have a double check here:
If we use the normal widget screen to set up widgets and then we go to the customizer we delete the legacy widget blocks, we add some blocks (paragraphs) and we press publish right away without saving a draft first, the saving does not happen.

It works as expected for me. But I think one changeset has to be taking precedence over the other. I don't think showing both edit options at the same time is a good idea. It will confuse users at best, and cause bugs at worst. Can we hide the legacy option if the new one is enabled?

87f2c37 Widget Blocks Manager: Define own noop function.

Loading

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Thank you for all the work, the iterations and diving deep on this feature 🥇. The initial objective was to have something experimental that we can test and iterate on, and I think we achieved this objective. This is a complex problem, and we have a good first step here.

I think we can merge (right after Travis is happy) and continue the development in other PR's 💥🎉.

Travis is identifying a lint problem on FILE: /app/lib/customizer.php. We should also update the "since versions" on some functions. Feel free to apply these changes and merge after.

For future reference/tests, I'm also writing here potential issues found during my tests. I don't think they are blockers.

Some issues I noticed that I think it is because we did not have yet a mechanism to handle the case:

  • When someone does some changes in the widgets tab (e.g., add a new widget) even if they are saved as drafts our block widget section does not use it.
  • When we preview another theme in the customizer, the standard widgets section updates to reflect the areas of the new theme but the new section doesn’t.

Things that may not be related/may be something on my setup/ plugins I have, etc:

  • There is a strange problem if we add blocks in a widget section and then we reload the old widget screen two times the blocks get removed, and some widgets get used (may not be related to this PR, previously a similar problem was happening but just when a changed was applied on the widget screen).
  • Adding blocks in the customizer on the 2014 theme may not work well. Problem described in more detail above; it works well when the area contains blocks, the problem only happens when the area contains standard widgets, and we are saving blocks for the first time (the problem seems intermittent).

Another issue I noticed is that if we add blocks to an area. We save everything; we reload the customizer we go to the standard widgets section; we delete the "Blocks Area" widget, and we try to re-add other widgets, the widget add does not work, and it is impossible for the user to re-add widgets using just the customizer. So if a user tries our experimental section and then wants to revert the only option is using the old widget screen, it is impossible using just the customizer, and some users may not be aware the widgets screen exist and use the customizer all the time. I guess this issue is the one with the most priority.

Loading

@epiqueras
Copy link
Contributor Author

@epiqueras epiqueras commented Jul 4, 2019

f1985b6 Customizer: Update @since version numbers.

Loading

@epiqueras epiqueras merged commit 05b9e95 into master Jul 4, 2019
4 checks passed
Loading
@epiqueras epiqueras deleted the add/widget-blocks-to-the-customizer branch Jul 4, 2019
@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 4, 2019

Congrats for your first PR @epiqueras 🎉

Loading

@youknowriad youknowriad removed this from the Future milestone Jul 5, 2019
@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 5, 2019
@jorgefilipecosta jorgefilipecosta moved this from Customiser UI to Non UI Issues/Tasks in Block-based Widgets Editor Jul 9, 2019
@jorgefilipecosta jorgefilipecosta moved this from Non UI Issues/Tasks to Done in Block-based Widgets Editor Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

5 participants