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

Site Editor: Implement a settings object filter #33737

Merged
merged 4 commits into from
Aug 3, 2021

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Jul 28, 2021

Potentially solves #33736

Description

The post editor has the block_editor_settings_all and block_editor_settings filter that allows us to modify the settings object injected into the block editor. A similar filter, however, doesn't exist for the site editor.

How has this been tested?

There should be no visible changes to the UI.

  1. Set up local Gutenberg dev environment
  2. Activate a block based theme
  3. Navigate to the site editor
  4. Smoke test the site editor to ensure that updates still persist correctly, block insertion still functions as expected, etc.

Screenshots

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Sorry, something went wrong.

The post editor has the block_editor_settings_all and block_editor_settings filter that allows us to modify the settings object injected into the block editor. A similar filter, however, doesn't exist for the site editor. We apply the same filter here for the site editor.
@jeyip jeyip added [Feature] Full Site Editing [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Jul 28, 2021
@jeyip jeyip self-assigned this Jul 28, 2021
@jeyip jeyip changed the title Add block editor settings filter for site editor init Site Editor: Implement a settings object filter Jul 28, 2021
Copy link
Contributor

@mattwiebe mattwiebe left a comment

Choose a reason for hiding this comment

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

Left a comment about needing another param and Core's filter documentation convention. Otherwise, this is great, and good call on not needing to call gutenberg_experimental_global_styles_settings directly, since it runs on the filter.

lib/full-site-editing/edit-site-page.php Outdated Show resolved Hide resolved
jeyip added 2 commits July 29, 2021 11:23
We need to add a second param to our call to apply filters because it is included in Core WordPress.
@@ -101,7 +101,10 @@ function gutenberg_edit_site_init( $hook ) {
'__experimentalBlockPatternCategories' => WP_Block_Pattern_Categories_Registry::get_instance()->get_all_registered(),
)
);
$settings = gutenberg_experimental_global_styles_settings( $settings );

$site_editor_context = new WP_Block_Editor_Context();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure why the edit site page doesn't integrate this filter like other screens. If we decide to change it, we should refactor the code to use gutenberg_get_block_editor_settings to handle it all behind the scenes rather that duplicating a similar logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I'll make the changes. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 72b175c

@@ -89,19 +89,17 @@ function gutenberg_edit_site_init( $hook ) {
*/
$current_screen->is_block_editor( true );

$settings = array_merge(
gutenberg_get_default_block_editor_settings(),
Copy link
Contributor Author

@jeyip jeyip Jul 30, 2021

Choose a reason for hiding this comment

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

gutenberg_get_default_block_editor_settings will be merged in the gutenberg_get_block_editor_settings function, which is why we remove it from here.

function gutenberg_get_block_editor_settings( $custom_settings, $block_editor_context ) {
$editor_settings = array_merge(
gutenberg_get_default_block_editor_settings(),
array(
'allowedBlockTypes' => gutenberg_get_allowed_block_types( $block_editor_context ),
'blockCategories' => gutenberg_get_block_categories( $block_editor_context ),
),
$custom_settings
);

@gziolo gziolo requested a review from youknowriad July 31, 2021 06:40
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code looks good and it aligns with configuration for other screens with the block editor.

@jeyip
Copy link
Contributor Author

jeyip commented Aug 2, 2021

Thanks @gziolo! Planning to let this PR sit a a few more days so that @mattwiebe and @youknowriad get a chance to take a look.

@youknowriad
Copy link
Contributor

If @gziolo is happy about multi editor settings, I'm happy too :)

Copy link
Contributor

@mattwiebe mattwiebe left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jeyip jeyip merged commit ac09a70 into trunk Aug 3, 2021
@jeyip jeyip deleted the try/apply-block-settings-filter-to-site-editor branch August 3, 2021 16:59
@github-actions github-actions bot added this to the Gutenberg 11.3 milestone Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing")
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants