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 preload_paths filter for widgets screen and full site editing #28701

Merged
merged 8 commits into from
Mar 11, 2021

Conversation

manooweb
Copy link
Contributor

@manooweb manooweb commented Feb 3, 2021

Description

This PR propose to fix #28690 by adding a filter for widgets screen and full site editing features to be able to preloading datas before the pages is loaded.

Edit: I've just updated this PR and the issue with a comment because I saw I had forgotten the navigation editor.

@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 3, 2021
@gziolo gziolo requested a review from a team February 4, 2021 10:50
@gziolo
Copy link
Member

gziolo commented Feb 4, 2021

Thank you for working on it. It feels like it's about time to abstract this editor setup to avoid code duplication but, more importantly, to prevent this type of issue.

@manooweb
Copy link
Contributor Author

manooweb commented Feb 4, 2021

Thanks!
Sure! Maybe Gutenberg itself could be use a filter because the paths couldn't be the same in function of the context (editor, widgets-editor...)
However, it also depends on how it will be merged later in the WordPress core.

@gziolo
Copy link
Member

gziolo commented Feb 4, 2021

We could always have someting like:

initialize_block_editor( array(
    'preload_paths' => array(
        // data
    ),
    // more shared settings
) );

Edit: I see there is gutenberg_get_common_block_editor_settings already that is a good start.

@gziolo gziolo requested a review from oandregal February 4, 2021 12:04
@talldan
Copy link
Contributor

talldan commented Feb 5, 2021

Thanks for working on this. Also related is #24642.

edit: I realise this doesn't add any paths for the navigation editor, just the preloading mechanism, so it doesn't actually resolve the above issue.

@talldan talldan added [Feature] Full Site Editing [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Enhancement A suggestion for improvement. labels Feb 5, 2021
@manooweb
Copy link
Contributor Author

manooweb commented Feb 5, 2021

Thanks for working on this. Also related is #24642.

edit: I realise this doesn't add any paths for the navigation editor, just the preloading mechanism, so it doesn't actually resolve the above issue.

Yes! for the navigation editor there was nothing when I worked so I had to implement just the preloading mechanism.
So I can continue to work on this PR:

Let me know.

@manooweb
Copy link
Contributor Author

manooweb commented Feb 5, 2021

We could always have someting like:

initialize_block_editor( array(
    'preload_paths' => array(
        // data
    ),
    // more shared settings
) );

Edit: I see there is gutenberg_get_common_block_editor_settings already that is a good start.

@gziolo we agree for the block editor, this is the responsability of WordPress core now ?
See https://github.com/WordPress/WordPress/blob/5.6.1/wp-admin/edit-form-blocks.php#L46-L83

The work in progress here could be also merged in WordPress core in a future release, right ?

// most of these are copied from edit-forms-blocks.php.
$preload_paths = array();

$preload_paths = apply_filters( 'navigation_editor_preload_paths', $preload_paths );
Copy link
Member

Choose a reason for hiding this comment

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

For the navigation screen, I see that there was an attempt to preload certain requests in #24672 (comment) but it didn't work well with the kind of entities managed by the navigation menu.

It looks to me that this PR could land without this part, which can be iterated later if/when we figure how to preload things for this editor.

@gziolo
Copy link
Member

gziolo commented Feb 5, 2021

We can have a follow-up PR with a new method that would get proposed to the core just after it lands in Gutenberg. That means your assumption is all correct.

@manooweb
Copy link
Contributor Author

manooweb commented Feb 16, 2021

As suggested by @gziolo #28701 (comment) I propose a modification to initialize each block-based editor the same way.

So I added the gutenberg_initialize_editor() function in editor-settings.php to put all is common for each of the editors including a filter for preload_paths.

I didn't deal with post block editor because, it seems to me that it isn't similar than the other, especially because of the way it is initialized:

I think we can add some other parameters passed to the new gutenberg_initialize_editor() function to be able to specialize more its code to the post block editor need.

For information, I confirm that the preload_paths filter is a real need for us as I described in the initial #28690 issue.

@draganescu draganescu mentioned this pull request Feb 18, 2021
62 tasks
Base automatically changed from master to trunk March 1, 2021 15:45
@manooweb
Copy link
Contributor Author

manooweb commented Mar 8, 2021

I rebased on trunk and corrected gutenberg_initialize_editor() documentation to respect coding standards which previously broke the PHP lint check job.

@gziolo gziolo requested review from aristath, mtias, mcsf and a team March 8, 2021 18:15
@manooweb
Copy link
Contributor Author

manooweb commented Mar 9, 2021

This PR only modify PHP code and doesn't touch anything about React Native.
Is there something I can do with React Native E2E Tests (Android) which break or is there just a problem on CI environment ?

@gziolo
Copy link
Member

gziolo commented Mar 9, 2021

This PR only modify PHP code and doesn't touch anything about React Native.
Is there something I can do with React Native E2E Tests (Android) which break or is there just a problem on CI environment ?

It looks like a general issue with the CI job. I see the same failure on trunk branch. I asked on WordPress Slack in #mobile channel for clarification (link requires registration at https://make.wordpress.org/chat/):
https://wordpress.slack.com/archives/C02RQC4LY/p1615271529010900

@hypest
Copy link
Contributor

hypest commented Mar 9, 2021

👋 all.

I see the same failure on trunk branch

Hey @gziolo , looking at the logs it looks like the fail here (timing out trying to load some gradle cache) was different than the ones on trunk (Cannot read property 'stopDriver' of undefined on a gallery block test).

I restarted the job here since it looks like an intermediate CI infra issue. We can investigate more if it persists with the same error. Sorry for the inconvenience!

lib/editor-settings.php Outdated Show resolved Hide resolved
lib/editor-settings.php Outdated Show resolved Hide resolved
@manooweb
Copy link
Contributor Author

manooweb commented Mar 9, 2021

Pushed again by taking in account the latest @swissspidy review comments. Thanks 😉

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

This tests well, and the code is a nice refactoring removing useless duplication, while also allowing for customizable preload_paths. From me, :shipit:

@draganescu draganescu merged commit e6d3f29 into WordPress:trunk Mar 11, 2021
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 11, 2021
@gziolo
Copy link
Member

gziolo commented Mar 12, 2021

Awesome work @manooweb, this is a great start towards unifying the way block editor is initialized on all pages 👍🏻

@manooweb
Copy link
Contributor Author

Thank you for merging 👍 and your encouragement ☺

@gziolo
Copy link
Member

gziolo commented Mar 24, 2021

I started looking into bringing this new helper into WordPress core. It looks like it might be quite a journey, see WordPress/wordpress-develop#1118 :)

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

@manooweb manooweb deleted the master branch April 8, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature widgets screen] a preload_path filter is missing
7 participants