[Block Editor - Inserter]: preload media categories empty check - client side #47503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Feature] Inserter
|
Size Change: +16 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
The |
I'm curious about this. If we can preload server-side it seems better no? |
|
Server-side preloading comes with the cost of increasing TTFB. I think it mostly depends on how critical the data is. |
|
TTFB is less critical in the editor (as opposed to frontend) but I can see the point. Seems we should gather all these preloads and have a unique way to perform them client site as well. We now have two of them: media and patterns |
|
Maybe we can have something similar to SWR's |
|
@youknowriad Is there no way for us to reasonably anticipate when the user will open the inserter / interact with a UI that needs the media categories (so that we could trigger the API requests early enough to not have a user visible delay)? The main concern about preloading is that we shouldn't preload something that we don't know for sure is needed. It's fine to preload things that would otherwise be requested on every editor load anyway, but for more specific use-cases I'd say we should be cautious, since every preload adds to TTFB. |
Not really. A user can just open the inserter as soon as the editor loads. |
|
@ntsekouras @youknowriad Taking a step back here, to understand the use-case better: Can you provide some context why we need to know media categories whenever the block inserter is opened? Shouldn't that only be relevant to using blocks that interact with media? My thinking is: If all I want to do with the inserter is e.g. insert a button block, or maybe I don't even want to use the inserter at all and just write some text, why do the media categories need to be loaded? |
|
@felixarntz In the new inserter, you can insert "media" directly and these are shown per category in the inserter |
ntsekouras
changed the title
|
@ntsekouras @youknowriad I just gave this a test to understand the concerns better. Here's my thoughts:
Does that make sense? Can we change the logic to always show the "Media" tab? This means we can just keep issuing these API requests only when the inserter is opened, rather than on page load, without any user-facing implications. |
|
@felixarntz the media tab could be empty if no media in Library and have the Openverse integration disabled with this setting. That would result in the Are these three requests for one item per media type so expensive that affect the performance? Maybe we could add a check about the Openverse setting mentioned above first and do the other requests async client side. That could probably work.. I'll try this out. |
ntsekouras
force-pushed
the
try/preload-media-categories-empty-check-request
branch
from
c7f761b
to
5043f2f
Compare
|
In my last commit the approach is:
|
|
Flaky tests detected in 545af30. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4230535508
|
ntsekouras
force-pushed
the
try/preload-media-categories-empty-check-request
branch
from
5043f2f
to
545af30
Compare
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
youknowriad
approved these changes
Feb 21, 2023
There was a problem hiding this comment.
Not perfect but works for me.
I think @felixarntz's remark is a good one in the sense that if we can accept "empty panels" with some kind of placeholder, we should be able to optimize the inserter a lot.
This doesn't apply only to the inserter, we do have similar UI optimizations for block switcher as well I think.
Anyway, that's more of a UX discussion: is it ok to show empty panels instead of removing the tab entirely.
ntsekouras
deleted the
try/preload-media-categories-empty-check-request
branch
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 42bba6e |
ntsekouras
added a commit
that referenced
this pull request
Feb 21, 2023
…ent side (#47503) * [Block Editor - Inserter]: preload media categories empty client side * show media tab if `enableOpenverseMediaCategory` setting is `true`
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
|
@ntsekouras @youknowriad Just thinking out loud here, I wonder if (for a future enhancement) we could potentially consider using the overall attachment count for this. Wouldn't having any attachment already satisfy the condition that at least one of the media categories is present, thus satisfy the criteria for showing the tab (together with the Openverse check)? If so, potentially we could use |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem
Right now these requests are preloaded here: https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.2/edit-form-blocks.php#L18.
In the back port PR for
6.2there were suggestions to do this client side: WordPress/wordpress-develop#3894 (review).I'm not sure how much we gain doing it here or this is the right approach TBH. I'll need feedback.
The other implementation I was considering was to do the requests in
EditorandSite Editorloading(ex: inuseBlockEditorSettings) something like this:...but when this API(setting) becomes stable, there would be no way to update this if that setting changes.
Testing Instructions