Site Editor: Improve loading experience (v2) #47612
Conversation
tyxla
added
[Status] In Progress
tyxla
requested review from
jsnajdr,
flootr,
youknowriad,
jameskoster,
mtias,
gziolo and
Mamaduka
| const SuspenseDataDependency = ( { store, selector, args = [] } ) => { | ||
| useSuspenseSelect( | ||
| ( select ) => select( store )[ selector ]( ...args ), | ||
| [] | ||
| ); | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
This is the component that we render for each declared data dependency. It will suspend when the data has not been resolved yet.
| { dataDependencies.map( | ||
| ( { store, selector, args }, depindex ) => ( | ||
| <SuspenseDataDependency | ||
| key={ `suspense-dep-${ depindex }-${ store.name }-${ selector }` } | ||
| store={ store } | ||
| selector={ selector } | ||
| args={ args } | ||
| /> | ||
| ) | ||
| ) } |
There was a problem hiding this comment.
Here's where we render one SuspenseDataDependency per data dependency.
| const contentDependencies = [ | ||
| // Global styles entity ID | ||
| { | ||
| store: coreStore, | ||
| selector: '__experimentalGetCurrentGlobalStylesId', | ||
| }, | ||
| // Global styles entity | ||
| globalStylesId && { | ||
| store: coreStore, | ||
| selector: 'getEditedEntityRecord', | ||
| args: [ 'root', 'globalStyles', globalStylesId ], | ||
| }, | ||
| // Menus | ||
| { | ||
| store: coreStore, | ||
| selector: 'getEntityRecords', | ||
| args: [ 'root', 'menu', { per_page: -1, context: 'edit' } ], | ||
| }, | ||
| // Pages | ||
| { | ||
| store: coreStore, | ||
| selector: 'getEntityRecords', | ||
| args: [ | ||
| 'postType', | ||
| 'page', | ||
| { | ||
| parent: 0, | ||
| order: 'asc', | ||
| orderby: 'id', | ||
| per_page: -1, | ||
| context: 'view', | ||
| }, | ||
| ], | ||
| }, | ||
| ].filter( Boolean ); |
There was a problem hiding this comment.
Here's where we specifically declare the dependencies for a suspense boundary.
Each dependency is represented by a store descriptor, selector and optional args to pass to the selector.
We'll need to tweak these a bit.
| @@ -173,7 +222,9 @@ export default function Editor() { | |||
| } | |||
| notices={ isEditMode && <EditorSnackbars /> } | |||
| content={ | |||
| <> | |||
| <LoadingScreen | |||
| dataDependencies={ contentDependencies } | |||
There was a problem hiding this comment.
Here's how we pass declared dependencies to the suspense boundary.
Technically, this gives us the flexibility to have multiple suspense boundaries that have different data dependencies.
|
Size Change: +745 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Here's what I'm seeing: loader.mp4There appears to be one loader in the frame/canvas, then another overlay the doesn't quite cover the entire viewport. Unsure if we need both? For a moment you're able to see the UI before the overlay appears which seems undesirable. |
|
Thanks @jameskoster, there's definitely more that can be done on the visual part. I did most of my testing in the edit canvas mode, but I've now done some additional polish so the overlay would cover all of the viewport area. FWIW the purpose of this PR was to demonstrate the mechanism to take advantage of In any case, it seems like with the different canvas modes, we'll still have to have a single spinner/loading screen instead of the initial plan to split into multiple sections that load separately. In the following days I'm planning to iterate and further polish the current state, which might mean at least:
Any other feedback is welcome, of course. Thanks! |
|
Flaky tests detected in 894d3d4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4055425813
|
There was a problem hiding this comment.
What's the reasoning for picking this approach over updating the blocks that make these requests instead?
I'm asking because I believe these requests can be dynamic: like template parts (we need the ids of the template parts), query blocks (we need the actual query which is dependent on the block config), the menus (menu ids)...
Thanks for asking. This was just another, more static and declarative approach that I wanted to get out of my brain and share with y'all. I realize the dynamic requirements and understand that it might end up being a blocker. However, this approach does have quite a few pros, too. It gives us complete control over the data we're interested in, which might be preferred in many cases. It also works without having to modify all of the components and blocks to use In any case, there are various angles we can approach this one from and I'd like to try a few different approaches before settling on a particular way forward. For example, another alternative could be recording all requests in some sort of request tracker, then waiting for all of them to resolve, and once they do, display the fully loaded editor. I'd like to tinker with that and see where it takes us. Maybe it will be a better compromise between staying flexible and dynamic and still not having to update all the blocks to specifically suspend rendering while loading. |
|
Personally, I've always felt like regular "Suspense" is the perfect use case for the editor loading. Isn't it an API that was specifically introduce to block rendering and orchestrate loaders. I think it's not important to solve everything in one go. Even if we start by just the template parts block, that would be a great start for me. My personal worry is on the "updates". In other words, if I use Do you have any ideas how they solve this kind of problem in React user land? In other words, can we have a "Suspense" loader that "removes" itself after the initial rendering? |
|
I guess according to React docs, the answer is to wrap our updates (inserting blocks, block attributes change...) with |
As I mentioned in my earlier comment, I have a separate idea of how to approach that problem. This behavior not supported by default in React, since in React, if we have components that suspend rendering, they'll keep suspending any time they are making a new request to retrieve data. That is expected to work well in the React world, but not when we only want to do it only for the initial loading of the app. So what we can do is a combination of two things: 1) having a "has initial loading finished" flag that starts as
Yes, that's what we intended to use, but that's not possible if we use |
tyxla
force-pushed
the
try/suspense-site-edit-2
branch
from
894d3d4 to
5444d21
Compare
tyxla
changed the title
|
I'm working on a more stable and flexible version of this in #50222. |
|
Since #50222 is much more reliable and flexible, I'm closing this one in its favor. |
What?
This is an experiment - DO NOT MERGE!
This PR aims to experiment with Suspense, specifically with our
useSuspenseSelect()implementation to load the site editor behind the scenes and display it once it's finished loading.This PR is an alternative to #42525.
Why?
We're aiming to find a way to improve the editor loading experience.
As @mtias says:
See #35503 for more details and motivation.
How?
We're building on what we've learned from #42525 and borrowing the loading screen from there.
We're also introducing a new concept of
SuspenseDataDependency. That is essentially a component that corresponds to a particular selector from a store that we know we're going to need somewhere in the app. That component will suspend when the data in question is still resolved. That allows us to use separate Suspense boundaries with separate data dependencies that we can specify declaratively.With that approach, we won't need to alter blocks and other components to use
useSuspendSelect.See my self-review for additional inline information.
This is obviously not landable in this version - we'll need to do a better job tweaking the data dependencies. I'd love some feedback on suggested dependencies.
E2E tests are also expected to fail right now - they don't know about this new loading screen 😉
Testing Instructions