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

Enable customizer legacy widgets only if set #16956

Merged
merged 3 commits into from Sep 3, 2019

Conversation

@tellthemachines
Copy link
Contributor

commented Aug 8, 2019

Description

Follow-up from #16626 . Hides the legacy widgets section in the customizer if legacy widgets are not enabled in the experiments settings.

How has this been tested?

Locally, by navigating to "Experiments" under the Gutenberg menu and toggling the "Enable Widgets Screen and Legacy Widget Block" checkbox. "Widget Blocks (Experimental)" section should display in Customizer when setting is enabled and Legacy Widget block should appear in the blocks menu.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
@youknowriad youknowriad requested a review from epiqueras Aug 8, 2019
@@ -40,6 +40,9 @@ export function initialize( id, settings ) {
*/
export function customizerInitialize( id, settings ) {
registerCoreBlocks();
if ( process.env.GUTENBERG_PHASE === 2 ) {
__experimentalRegisterExperimentalCoreBlocks( settings );

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 8, 2019

Contributor

Why was this not here before? How was the legacy widget block being registered?

cc @jorgefilipecosta

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 8, 2019

Author Contributor

Initially it was being registered in registerCoreBlocks along with everything else. I moved all the experimental blocks out in my previous PR #16626 so we could enable/disable them more easily.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 8, 2019

Contributor

Got it. Wouldn't that break the screen in the meantime though? If someone loads the Customizer, they won't have the legacy widget block registered right now?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 9, 2019

Author Contributor

Yup, it's looking a bit broken now unless you have the setting enabled 😅

'gutenberg_widget_blocks',
array( 'title' => __( 'Widget Blocks (Experimental)', 'gutenberg' ) )
);
}
$wp_customize->add_control(

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 8, 2019

Contributor

Shouldn't we bail out of adding anything related to the widget blocks?

Adding the controls and filters without the section might still cause issues.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 8, 2019

Author Contributor

Good point, I'll move it inside the condition.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 9, 2019

Contributor

It's not just the section and controls though.

There are a lot of filters and actions there that modify legacy widget logic. Are we sure we are not breaking any expectations by leaving them there. Can we not just not load any file related to the customizer in load.php?

cc @jorgefilipecosta

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 14, 2019

Author Contributor

Hmm, that doesn't work. Loading customizer.php conditionally causes the whole customizer page to crash when the widget experiment is disabled. The absence of add_filter( 'widget_customizer_setting_args', 'filter_widget_customizer_setting_args' ); is what does it, though I have no idea why. Not loading widgets.php also causes problems when the experiment is disabled: the page loads with a warning that $sidebar_widget_ids in class-wp-customize-widgets.php is the wrong type. That one seems to be caused by the absence of add_filter( 'sidebars_widgets', 'Experimental_WP_Widget_Blocks_Manager::swap_out_sidebars_blocks_for_block_widgets' ); 🤷‍♀
In any case, it's better to load customizer.php by default because even though it's only being used for widget-related stuff now, that might change in the future, so isolating the widget logic within it now will save us work later.
I'd still like to know why not loading these filters causes everything to explode though 😁 . Maybe @jorgefilipecosta can shed some light on that?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 14, 2019

Contributor

It's because the filters work together to change widget areas from storing an array of widgets to store a post ID. That's why I was thinking we should disable loading the entire files. I'm not sure if leaving some of the filters active could cause some weird side effects.

I guess that if we test it well enough and @jorgefilipecosta doesn't see any problems from his side then I'm happy to merge this.

@@ -69,12 +69,16 @@ function gutenberg_widgets_init( $hook ) {
);
}
$experiments_exist = get_option( 'gutenberg-experiments' );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Aug 20, 2019

Member

Hi @tellthemachines instead of replicating logic from function gutenberg_experiments_editor_settings here could we just call it on return?

return gutenberg_experiments_editor_settings( $settings );

This would also take care of enabling navigation on the widgets screen, would remove duplicate code.

I followed this approach in #17005.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 23, 2019

Author Contributor

Done!

@tellthemachines tellthemachines force-pushed the fix/customizer-experimental-widget-blocks branch from 8b64255 to 1c3495b Aug 23, 2019
Copy link
Member

left a comment

Everything worked well on my tests 👍

@tellthemachines tellthemachines merged commit a400f7b into master Sep 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@tellthemachines tellthemachines deleted the fix/customizer-experimental-widget-blocks branch Sep 3, 2019
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* Enable customizer legacy widgets only if set

* Only add control if experiment enabled.

* Remove unnecessary repetition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.