Skip to content

Add preload paths for media categories#3894

Closed
ntsekouras wants to merge 1 commit intoWordPress:trunkfrom
ntsekouras:backport/media-categories-preload-paths
Closed

Add preload paths for media categories#3894
ntsekouras wants to merge 1 commit intoWordPress:trunkfrom
ntsekouras:backport/media-categories-preload-paths

Conversation

@ntsekouras
Copy link
Copy Markdown
Contributor

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

This PR adds the new preload paths for the inserter media categories for post and site editors.
Related GB PR: WordPress/gutenberg#46251


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

array( '/wp/v2/settings', 'OPTIONS' ),
'/wp/v2/media?context=edit&per_page=1&orderBy=date&media_type=image',
'/wp/v2/media?context=edit&per_page=1&orderBy=date&media_type=video',
'/wp/v2/media?context=edit&per_page=1&orderBy=date&media_type=audio',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's preferred in this format:

add_query_arg(
	array(
		'context'    => 'edit',
		'per_page'   => 1,
		'orderBy'    => 'date',
		'media_type' => 'image',
	),
	rest_get_route_for_post_type_items( 'attachment' )
),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah this would be better.

Copy link
Copy Markdown
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This change added three somewhat complex database queries per page. Can please try and avoid this and just load them in the browser.

Screenshot 2023-01-24 at 12 40 39

Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ntsekouras I see this implemented in Gutenberg, however in terms of performance considerations I would like to understand why these requests are being preloaded. Will they be made anyway as soon as the block editor loads?

We're talking about WP Admin here, so performance is not as critical as for the frontend, but I would still want to ensure we don't add these queries unnecessarily.

Since I don't have the context here, can you outline the use-case for what these endpoints are used? And most importantly: Is this relevant for every block editor page load, or do you have to do something specific in the block editor for that data to become relevant?

@ntsekouras
Copy link
Copy Markdown
Contributor Author

ntsekouras commented Jan 25, 2023

@ntsekouras I see this implemented in Gutenberg, however in terms of performance considerations I would like to understand why these requests are being preloaded. Will they be made anyway as soon as the block editor loads?

@felixarntz yes, they need to, and they are part of the inserter media tab. When a user opens the inserter we need to make these requests to check if there is at least one media item per category(audio, image, video). If there is no result we don't need to show the inserter menu item for that category.

If we don't make these requests, the media tab would need to wait for the resolution of these calls, making it display afterwards. This can be a jarring experience and we want to avoid it.

The media inserter tab is implemented for post and site editors.

@TimothyBJacobs
Copy link
Copy Markdown
Member

I agree with @felixarntz and @spacedmonkey.

If we don't make these requests, the media tab would need to wait for the resolution of these calls, making it display afterwards. This can be a jarring experience and we want to avoid it.

I don't think these need to be completed when the editor is initially loaded. Instead, they could be loaded asynchronously when the editor loads, but before the user opens the inserter.

@hellofromtonya
Copy link
Copy Markdown
Contributor

I don't think these need to be completed when the editor is initially loaded. Instead, they could be loaded asynchronously when the editor loads, but before the user opens the inserter.

Given this feedback, what needs to change in this PR? @ntsekouras is this something being worked on?

@felixarntz
Copy link
Copy Markdown
Member

@hellofromtonya I believe this is currently being discussed further in WordPress/gutenberg#47503.

@ntsekouras
Copy link
Copy Markdown
Contributor Author

Given this feedback, what needs to change in this PR? @ntsekouras is this something being worked on?

@hellofromtonya I've opened a GB PR to see if it makes any difference to preload these client side in JS. The discussions there are mostly about whether we can avoid making the requests as soon as the editor loads, but I don't think that's an option.

This is because these requests are needed for a user before opening the inserter, which is something that could be their first actions as soon as the editor loads..

@hellofromtonya
Copy link
Copy Markdown
Contributor

Thanks @felixarntz and @ntsekouras for the update on where the discussion is happening.

@ntsekouras
Copy link
Copy Markdown
Contributor Author

I'm closing this since we'll back port the following changes here: WordPress/gutenberg#47503 which handle this issue client side.

The approach is:

  1. If enableOpenverseMediaCategory setting is set to true(which is the default) show the media tab and also init the requests for the media types in library. This ensures that the media tab will be there right after we open the inserter and since it's not the default tab for any editor, it will almost certainly have resolved the requests when a user selects that tab.
  2. If enableOpenverseMediaCategory is set to false, the media tab will wait for the resolution of the requests and will be displayed if there are any available media categories. This is not ideal, but it's less common and it's better than showing the media tab where there could be a chance to remove that tab after the requests' resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants