Site Editor: Improve loading experience (v3) #50222
Conversation
tyxla
added
[Status] In Progress
|
Size Change: +159 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
|
I like the simplicity of this and I do see us potentially using it for the initial loading in the site editor but unfortunately, it has one major drawback. We can't know which block editor instance triggered the requests, so we can't integrate it into patterns loading / templates loading... and that's also an important use-case IMO. |
|
Flaky tests detected in c5467c9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4934039438
|
Could you please provide more details about the flow you're referring to? From my understanding, this loading experience should be usable for editing templates as well, but I assume you mean something else when you say "templates loading" and "patterns loading". Ideally, step-by-step instructions would be very helpful to ensure we're talking about the same thing. |
|
@tyxla There are multiple possibilities:
Does that help? |
|
@youknowriad thanks, it does help! The intention of this PR isn't to cover all those cases, though. We'd be happy to explore solutions for improving their loading experiences separately. |
|
All tests are now green and I think this is ready for a review at this point. I believe this is far from perfect, but to a degree fulfills our initial intent. Any feedback is welcome! |
tyxla
requested review from
mtias,
jsnajdr,
flootr,
youknowriad,
jameskoster,
gziolo,
glendaviesnz and
kevin940726
tyxla
added
Needs Design Feedback
|
Thanks, everyone for the excellent design feedback! Any other feedback on the code before we go ahead with this one? |
| } | ||
| }, [ loaded, hasResolvingSelectors ] ); | ||
|
|
||
| const isLoading = ! loaded || ! hasLoadedPost; |
There was a problem hiding this comment.
Potentially this logic to compute the isLoading const could be extracted to a custom hook useSiteEditorLoading or something.
There was a problem hiding this comment.
While I don't see the immediate need for that, I'm happy to go ahead and extract it when it can actually be reused somewhere.
There was a problem hiding this comment.
it is mostly a suggestion to keep that logic outside the Editor component (timeout...) But I don't feel strongly.
There was a problem hiding this comment.
I think this is a good idea, I'll follow up in another PR.
| * | ||
| * @return {boolean} True if one or more selectors are resolving, false otherwise. | ||
| */ | ||
| export function hasResolvingSelectors( state ) { |
There was a problem hiding this comment.
Depending how confident we are with this selector (it seems fine to me), we could consider making it private to start with.
There was a problem hiding this comment.
To me, one of the positive sides of metadata selectors is that they are always publicly available. That being said, I wonder if you think there's a particular need for this specific one to be private.
There was a problem hiding this comment.
Not for me. I think it's fine for this one to be public. I was suggesting it in case we missed confidence.
tyxla
force-pushed
the
try/site-edit-loading-experience-3
branch
from
c7fed44
to
c5467c9
Compare
|
Thanks, everyone! Looking forward to adding more improvements to the loading experience in the near future as part of #35503. |
|
Thanks for this one! 🙏 |
| return [ ...Object.values( state ) ].some( ( selectorState ) => | ||
| [ ...selectorState._map.values() ].some( |
There was a problem hiding this comment.
-
Are the copies necessary?
[ ...Object.values( state ) ].some->Object.values( state ).some, and likewise forselectorState. -
After digging around a bit, it seems like
._mapis an internal property of packageequivalent-key-map. Right? My follow-up questions are: 1) is this the right/only way to consume it? 2) If so, shouldn't we at least have a comment explaining how and why we are using a private interface of an external package?
There was a problem hiding this comment.
The spread operators would be needed for results of Map.prototype.values(), because that's an iterator. But Object.values() returns an array, and that doesn't need the extra [...]. So, one of the spreads is justified, the other is not.
The .map.values() call indeed depends too much on the implementation details, and it seems to me it works only when the selector has exactly one argument? Because then the value is not in resolution[ 1 ], but deeper in the tree. But the only remedy is to contribute a new values() method to equivalent-key-map itself.
There was a problem hiding this comment.
Thank you for the feedback @mcsf! I've addressed it in #50865. A couple of notes to add:
-
The first copy was indeed unnecessary, but the second one is necessary since the returned iterable iterator object doesn't have a
.some(), thus we need to convert it to an array. We could use theEquivalentKeyMap'sforEach()method, however, this would be less optimized since it often will require more iterations than a.some(). -
I've added a comment to explain that usage. As mentioned above,
.some()is a more optimized usage since it doesn't require iterating through all elements, which is why I prefer it to the.forEach()thatEquivalentKeyMapsupports.
Alternatively, we could open a PR in the EquivalentKeyMap repo for supporting .some() or a method to just retrieve all elements from the map if you think that will help.
Edit: ha, Jarda beat me to it. Spot on btw!
There was a problem hiding this comment.
Note that the standard Map doesn't support .some. Only .keys, .values, and .entries, and unlike the similar Object.* methods they return iterators. Only after converting to array with [...] or Array.from you can call .every or .some.
What we'd like to do is to add .keys, .values and .entries to EKM. And I also suspect that their (or, more precisely, Andrew's 🙂) .size implementation is wrong, because it returns ._map.size, but the ._map is a tree with potentially many nested leaf nodes.
There was a problem hiding this comment.
Thanks for the answers, both of you. Right, I didn't take the time to understand the data structure behind selectorState, and indeed .values in the context of a map should just be an iterator.
As mentioned above, .some() is a more optimized usage since it doesn't require iterating through all elements, which is why I prefer it to the .forEach() that EquivalentKeyMap supports.
To be clear, I think .some is the best idiom for the situation, and I wouldn't want to replace it with .forEach.
I'm fine with just removing the copy around Object.values and leaving the other as is. However, just a minor question: personally and intuitively, do you make a difference between [ ...xs ] and Array.from( xs )? For me, I tend to infer that the purpose of the former is to copy, while that of the latter is to cast (from iterator to array). If I'm the only one with this intuition, then ignore me; if not, then maybe this piece of code becomes 0.1% clearer by using Array.from( selectorState._map.values ).some. 😉
There was a problem hiding this comment.
TBH spread and Array.from are all the same for me in the current use case (they do have some differences, where Array.from() is a bit more powerful and supports more use cases than iterables, but here it doesn't really matter.
Regardless, I'm happy to make this 0.1% more readable 👍 Updated in the PR.
There was a problem hiding this comment.
I usually prefer Array.from, because it does the same thing, but is less obscure: it's a regular function call, not a special syntax. Spread operator is more useful when spreading multiple things together.
What?
This experiments with loading the site editor behind the scenes and displaying it once it's finished loading. This time, we're doing it by checking whether we have any resolving selectors belonging to the core data store.
This PR is an alternative to #42525 and #47612, but this time, we don't utilize Suspense.
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 #47612. We're reusing the existing loading screen.
Demo - zoomed-out mode loading
Screen.Recording.2023-05-02.at.16.16.10.mov
Demo - editor canvas loading
Screen.Recording.2023-05-02.at.16.19.15.mov
Testing Instructions