Block preview: build in async rendering #60425
Merged
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -149 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
|
In list layouts, the spinner doesn't seem to be centered properly. |
|
To be honest, I'm not entirely sure the spinner adds value because it's usually a very short time so it feels like a glitch. |
youknowriad
approved these changes
Apr 4, 2024
|
This PR seems to improve the "navigate" metric in the site editor significantly. |
Agree. The block previews are queued to render all at once that the spinner isn't helpful in this case. I'd remove it. Thanks for testing it. I'm happy to go with this approach and iterate, though this PR worsens interactivity while improving the first load — I presume more async rendering makes the browser trigger the metric we use for
Gravacao.do.ecra.2024-04-04.as.11.00.08.mov
Gravacao.do.ecra.2024-04-04.as.11.05.02.movI think async rendering fields is a better fit for DataViews than batching the whole list. However, I am concerned about the interactivity so I wouldn't remove the Ideally, we'd batch the async rendering of previews, that'd be the best of both worlds. |
oandregal
reviewed
Apr 4, 2024
oandregal
reviewed
Apr 4, 2024
|
@oandregal looking at your videos, I'm not entirely sure I understand why this PR would be less "interactive" than trunk. The only possible explanation that I can think about is that in trunk you're actually filtering before all the previews render which means obviously filtering is faster because there's only a small number of previews rendered while in this PR, it's a bit more performance so it takes less time for the previews to render which means filtering while a bigger number of previews is rendered is "less interactive". Meaning that in trunk if you want a bit more (wait for all the previous to appear) and filter should be very similar in terms of "interactivity" to this PR. |
|
@youknowriad I think I found the issue. I was not using |
youknowriad
reviewed
Apr 4, 2024
|
|
||
| useEffect( () => { | ||
| const context = {}; | ||
| blockPreviewQueue.add( context, () => { |
There was a problem hiding this comment.
wild idea not sure if good.
What if we have an argument to the "add" function that says even if there's still time remaining, schedule an update on the next idle callback window.
There was a problem hiding this comment.
Basically the goal: to give more time for each template to render before moving to the next, even if the template itself has some small async stuff (like useEffects...)
There was a problem hiding this comment.
Right you mean only one call per idle callback? I wonder if we'd still need to do the time remaining check though, maybe the browser just calls those idle callbacks in sync? I haven't tested, so not sure.
There was a problem hiding this comment.
yes, it could be a "config" option as well when creating the queue.
There was a problem hiding this comment.
I'll suggest this in a follow up because it will touch async list as well and we'll need to do a new round of testing. But at first glance it look to me like it solves the same issue. I'll create a new PR after this and also loop in Jarda
|
Nice! I've tested with a 6x slowdown and making grid the default for templates. I cannot say which one (this PR or trunk) performs better in terms of interaction — that's good. And this PR renders all items at once, allowing you to scroll, etc. This PR: Gravacao.do.ecra.2024-04-04.as.13.15.33.movGravacao.do.ecra.2024-04-04.as.13.14.38.movTrunk: Gravacao.do.ecra.2024-04-04.as.13.20.10.movGravacao.do.ecra.2024-04-04.as.13.19.33.mov |
oandregal
approved these changes
Apr 4, 2024
|
Fantastic work; I enjoyed reading through the discussions and code. Did we just re-invented React |
|
@Mamaduka Indeed, this is something that could be built-in by default in React. A way to give "priorities" to some components. I think some of React's 19 features go into this direction (Offscreen...) but there's not much details so far. Let's see. |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: ellatrix <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: oandregal <[email protected]>
This was referenced Nov 4, 2024
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.
What?
This PR builds in async rendering for the BlockPreview component, which allows us to remove the
useAsyncListuses. The benefit of this is that a whole list of items that include previews can be rendered upfront, while previews inside of them are now rendered async. This gives us a less jarring experience in the template and patterns grid for example.Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast