Site Editor: Improve loading experience (v1) #42525
Conversation
tyxla
added
[Status] In Progress
|
Size Change: +619 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
Very excited to see some work on this! :) I'm unable to explain why, but I am seeing small flashes of content behind the loader: load.mp4Let me know how I can help debug that. It's generally not recommended to use a logo for a loading graphic as it can create a negative association with the brand. I'm not sure how much truth there is to that, but it might be best to use something like the Spinner component for now anyways? Hopefully in the future we can use a determinate progress bar. |
|
This is great, thanks for working on it! It'll have a significant impact on the perception of the editor and the fragility waterfalls tend to signal. |
|
@tyxla, impressive work so far. It shows the power of new React APIs ❤️ Have you experimented with giving priorities to suspense calls so we could render some more critical chunks of the page faster? |
|
Thanks for your feedback everyone!
Yes, those are easy to reproduce with CPU throttling. Those flashes will generally occur in periods when there are no async requests in flight, and our suspense boundary "thinks" that loading has finished, thus no longer presents us with the fallback. However, if this happens in the middle of the loading, when a new request is issued shortly, the fallback is displayed again. This is a downside of the lack of control we have over the current mechanism, and demonstrates the need to create a centralized mechanism that allows for better granularity and control (cc @griffbrad as we've been discussing that before). I've also demonstrated this in a simpler and isolated way in #42188. Alternatively, we could hack around this by debouncing the rendering with a small delay, however there's no guarantee it'll cover the flickering, because how short (or how long) a period without requests in the middle of the loading experience can vary greatly.
Of course, this was just for experimentation and demonstration purposes and never meant to land as-is. I've replaced the logo with a spinner component in the meantime if that helps.
No, I haven't. Actually, I'm not sure this makes a lot of sense for a few reasons:
Of course, I might be misunderstanding, so let me know if that's the case. |
@jameskoster I think this is kind of nonsense :D
Correct. We might want to identify some structural parts like editor header and navigation bits (W menu) first and display those as soon as ready so you can go elsewhere and not wait for everything to load before navigating. |
Oh, interesting. I'd love to see some sort of a raw preview or demo of how you envision this working so I could tinker with it. |
Maybe :D
A very rough idea:
Note how the W button finishes loading first, then the top bar, then the canvas. I do wonder about detaching the top bar / canvas though. What happens if you open the Inspector before the canvas is loaded? Or try to add a block via the Inserter? I'm not sure there's any real benefit to loading those tools before the canvas. It might be better to simply load those together. An alternative:
|
tyxla
force-pushed
the
try/suspense-site-edit
branch
from
f936895 to
523662a
Compare
|
Thanks for the demo! At a first glance, this would be a bit troublesome with the current interface skeleton structure. Let me clarify. Currently, we have this rough structure for the site editor:
To achieve what's suggested on the video, we'd have:
However, for this to work, we need to have no async suspense select calls outside of suspense boundaries. This is tough, because the Editor and the Header, for example, will share some data dependencies, and we can't (and shouldn't) query those data dependencies both ways. To be able to do that, we'd need to structurally pull out the header from the interface skeleton, so it would be outside of the Editor, in order for the Editor and the Header to have separate suspense boundaries. We should be able to get around this with That being said, I wonder what still prevents us from upgrading to React 18 and if we can push that forward. cc @gziolo |
We also probably won't need to use suspense select everywhere. Maybe start with selectors that fetch entities from the server. |
|
@tyxla @jameskoster we don't need anything too complicated at first here. Rendering a very basic skeleton (without pulsing elements) just outlining the header and site icon slot is fine. More important to get the foundation in so we can refine. |
|
Here's this PR as things stand:
It's already in a decent spot imo, and potentially mergeable assuming subsequent refinements. If it's easy to try, we could use a slightly larger spinner, and print a very simple indication of where the site logo / top bar will appear:
|
|
Yep, that last one is what I had in mind |
|
One issue to fix: The loading state should (for now) only be displayed when you first open the Site Editor. At the moment I noticed it pops up when you perform actions like:
|
tyxla
force-pushed
the
try/suspense-site-edit
branch
2 times, most recently
from
5784e42 to
ebe9ce1
Compare
|
Thanks for the feedback everyone! I've done some changes and here's how it looks now: Screen.Recording.2022-08-17.at.15.25.19.movBasically, the header is now outside and since it generally loads quickly and doesn't have a ton of dependencies, I don't see a reason to add suspense to it at this time. We could, of course, do it if we find a good use case that makes it load slowly. Beyond that, I've moved the boundary for the content and since will take more time to load, we'll see the spinner. I've increased the size of that a bit, just to demonstrate how things look that way. Let me know what you think!
I'd be happy to tinker with that, but was curious - what happens when we start interacting with the app and new data gets requested? Is it going to work just like it did before - with placeholders and spinners all over the place? |
|
@ndiego This work is unfortunately still very experimental and not likely to be in 6.1. You can safely remove it from any 6.1 project tracking stuff. |
ndiego
removed
the
Backport to WP 6.7 Beta/RC
|
Thanks for the clarification @jsnajdr, removing the backport label. |
Indeed. One way around this would be having a mechanism to always decide on runtime whether to use |
After reading a bit on the internet (as this seemed like a common issue people could face), it seems the solution for folks here is to actually use "startTransition" to avoid showing the fallbacks. https://reactjs.org/docs/react-api.html#starttransition So is it possible to have this:
What would be the issues with such approach? |
|
(I guess we need to wait for React 18 for startTransition to be available) |
|
I've been hoping to use Which means the upgrade to React 18 will be unblocked very soon and we can reattempt it. |
|
Where are we on the update to 18 blocker? |
const { setAttributes } = useDispatch();
startTransition( () => {
setAttributes( newAttributes );
);the I don't fully understand the problem we hope to solve with |
|
Some context on what @jsnajdr talks about above can be found in this discussion: reactwg/react-18#86 Particularly this:
I don't think we considered experimenting with I'm starting to doubt if with suspense we will be able to solve this problem altogether. The issue there is all the third-party blocks and plugins which will not be suspending rendering in their components. We can't realistically ask for every plugin to suspend, therefore, we might need an alternative implementation. This brings me back to an alternative solution we discussed earlier in the summer with @jsnajdr - basically implementing a custom wrapper system that relies on the status of the resolvers we're interested in and based on that, rendering either the placeholders or the actual component. @jsnajdr @youknowriad do you have any additional thoughts or ideas on moving forward with this one? |
Personally, I don't see this as a blocker at all. The important thing is that we get "some loaders" absorbed by the suspense provider, not all of them. Blocks like reusable blocks, template parts and navigation blocks are the most important one here, maybe images as well but if there's some blocks including third-party blocks that load asynchronously (have their own loaders), that's totally fine. |
|
I agree — we don't need to absorb everything for this to succeed in addressing the main problem. It's ok to have a few things here and there waterfall loading; it's not ok for the main pieces of the interface and fundamental building blocks to load uncoordinated. Let's please start small and make the initial experience better than what it is today while we figure out the more difficult parts and how they integrate with the block API for other authors to take advantage of. |
|
Thanks for the feedback, everyone! I'm working on another experiment today - similar to what we had here, but a bit simpler. My idea is to still use suspense but to try declaring dependencies per the suspense boundary. That will not only give us the required granularity to add multiple suspense boundaries (header, content, footer) but will also allow us to have the flexibility and add/edit data dependencies for each suspense boundary as needed. PR coming soon. |
|
Here's the PR of this other experiment I was referencing above: #47612 |
tyxla
changed the title
|
I've started working on a v3, which I prefer the most from the 3 approaches: #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.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 essentially replacing a bunch of
useSelect()withuseSuspenseSelect, adding aSuspenseboundary with a dummy loading screen, and some artificial delay to prevent from flickering.This is obviously not landable in this version - we'd likely need a better and more selective approach to which
useSelectto alter and which not.Testing Instructions
Screenshots or screencast
Screen.Recording.2022-07-19.at.14.52.31.mov