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.

Sign up for GitHub

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

Improve Nav block loading UX by preloading Navigation menu requests #48683

Merged
merged 12 commits into from
May 11, 2023
Merged

Improve Nav block loading UX by preloading Navigation menu requests #48683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8f0e60c
Scaffold out preloading
getdave Mar 2, 2023
17a1ae7
All GET and OPTIONS preloading
getdave Mar 8, 2023
91d7f84
Make preload paths more readable via add_query_arg
getdave Mar 8, 2023
aa9555c
Add comment to unusual usage
getdave Mar 8, 2023
8353824
Resolve PHPCS
getdave Mar 8, 2023
24bd56d
Fix formatting
getdave Mar 8, 2023
b188a19
Rename away from “permissions”
getdave Mar 8, 2023
c28dcb2
Limit to Site Editor
getdave Mar 8, 2023
33ea7e6
Add context to doc block
getdave May 11, 2023
4e58c8b
Preload Browse Mode sidebar Navigation
getdave May 11, 2023
615ec2e
Remove redundant preload
getdave May 11, 2023
f2ab9dc
Use int not string for numeric
getdave May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions lib/compat/wordpress-6.3/navigation-block-preloading.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php
/**
* Patches resources loaded by the block editor page
* to include Navigation posts.
*
* @package gutenberg
*/

/**
* Preloads requests needed for Navigation posts
*
* @param array $preload_paths Preload paths to be filtered.
* @param WP_Block_Editor_Context $context The current block editor context.
* @return array
*/
function gutenberg_preload_navigation_posts( $preload_paths, $context ) {
Copy link
Member

Choose a reason for hiding this comment

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

This implies that regardless of the theme and current setup of the site, the site will always have a navigation menu. I think that's a false assertion - maybe the theme doesn't provide a navigation by default, or maybe the user has currently removed it from their site, or used a different block to set a navigation menu up. That way we'll always preload the wp_navigation posts, regardless of whether they're necessary.

Just imagine if the previous theme supported those and the site has a ton of those navigation posts, but the current theme doesn't make use of them. That will preload all those posts in vain.

Could be a longshot, but I was wondering if we could add some sort of a check to ensure that these navigation posts are actually going to be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implies that regardless of the theme and current setup of the site, the site will always have a navigation menu. I think that's a false assertion...

That's certainly an argument that can be made. On the other hand you're probably talking about an 20% use case, with the other 80% of sites being almost certain to be using some kind of Nav menu.

That way we'll always preload the wp_navigation posts, regardless of whether they're necessary.

My question is how bad is it exactly for those 20% users?

Just imagine if the previous theme supported those and the site has a ton of those navigation posts, but the current theme doesn't make use of them. That will preload all those posts in vain.

This is certainly valid but very much an edge case IMHO. How does this weight against the majority of users who will experience a poor initial editor experience due to interdependent, network requests running in sequence and blocking the initial "load" state of the block (and thus the editor as a whole)?

Could be a longshot, but I was wondering if we could add some sort of a check to ensure that these navigation posts are actually going to be necessary?

Yes ideally we would do that. But I cannot see a way to do that without making more queries which is then self defeating.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation. That makes sense to me 👍 It's a compromise and it seems like it's better to have the optimization than to not have it. Would be nice to see some measurements and statistics that confirm the 80% and 20% assumptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would be useful. Obviously I'm generalising heavily from intuition only 😅

I wonder how we could determine stats on how many Themes include a Nav block. Maybe @MaggieCabrera or @scruffian would have an idea?

Copy link
Member

Choose a reason for hiding this comment

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

It definitely would be hard to say 😉 Especially given that one could install a plugin for their menu (like one of those megamenu plugins) and not use the Nav block at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth noting that it's possible to disable preloading by filtering on block_editor_rest_api_preload_paths and removing all requests that contain wp/v2/navigation. So it's not something folks are locked into. But a non-developer user would find this difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

most sites will have a Nav block.

I think this is a safe assumption, also keep in mind that fresh installs have tt3 on them, which has the block on the theme.


// Limit to the Site Editor.
if ( ! empty( $context->name ) && 'core/edit-site' !== $context->name ) {
return $preload_paths;
}

$navigation_rest_route = rest_get_route_for_post_type_items(
'wp_navigation'
);

// Preload the OPTIONS request for all Navigation posts request.
$preload_paths[] = array( $navigation_rest_route, 'OPTIONS' );

// Preload the GET request for ALL 'published' or 'draft' Navigation posts.
$preload_paths[] = array(
add_query_arg(
array(
'context' => 'edit',
'per_page' => 100,
'_locale' => 'user',
// array indices are required to avoid query being encoded and not matching in cache.
'status[0]' => 'publish',
'status[1]' => 'draft',
),
$navigation_rest_route
),
'GET',
);

// Preload request for Browse Mode sidebar "Navigation" section.
$preload_paths[] = array(
add_query_arg(
array(
'context' => 'edit',
'per_page' => 1,
'status' => 'publish',
'order' => 'desc',
'orderby' => 'date',
),
$navigation_rest_route
),
'GET',
);

return $preload_paths;
}
add_filter( 'block_editor_rest_api_preload_paths', 'gutenberg_preload_navigation_posts', 10, 2 );
1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require_once __DIR__ . '/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php';
require_once __DIR__ . '/compat/wordpress-6.3/rest-api.php';
require_once __DIR__ . '/compat/wordpress-6.3/theme-previews.php';
require_once __DIR__ . '/compat/wordpress-6.3/navigation-block-preloading.php';

// Experimental.
if ( ! class_exists( 'WP_Rest_Customizer_Nonces' ) ) {
Expand Down