DataViews: Add loading indicator for preview field #61408
Closed
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
t-hamano
added
[Type] Enhancement
|
Size Change: +12 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
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. |
|
We actually had this exact same code in the initial PR that introduced the Async component but we decided to remove it because we felt that the loading time was too small and just caused a feeling of things jumping around for not reason. So let's try to get some more opinions |
|
I believe the original issue that this PR is trying to fix is different though. It's not about the loading indicator of the "Async" component, it's more about finding a way to delay the rendering of the preview until all the blocks that have async behavior has loaded (template parts, query loops, navigation block, images...). This will be harder to fix and only possible with something like "Suspense" (or some custom version of it) |
|
On high-end devices, the loading indicator adds noise, in my view. Though having it for lower-end devices could perhaps improve the perceived experience, as you have some info about what's going on. This is how it behaves with a 4x slowdown: Gravacao.do.ecra.2024-05-07.as.10.00.34.mov |
|
Thanks for the feedback! If we feel this indicator to be noisy on general devices, I would like to close this PR. Regarding #59832, we may need to take a different approach. |
That's exactly it. We're basically looking to achieve the same loading effect as the main preview Frame, IE the full preview appears as a single unit with no jumping: loader.mp4 |
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.
Fixes #59832
What?
This PR adds a loading indicator to the data view preview field.
Why?
See #59832
How?
Thankfully, the
Asynccomponent added by #60425 supports theplaceholderprop, so we can just pass in the component we want to display while loading and it seems to work as expected.Also, this PR simply renders the Spinner component, but in table layouts it may be a good idea to set a minimum height for the preview area.
Note: Looking at the discussion around #60425, it appears that the Spinner component was once added, but eventually removed. In the current DataViews, I think it makes sense to display a Spinner component, but I would like to know what you think about this.
Testing Instructions
Try reloading your browser or switching layouts on pages that use DataViews.
Screenshots or screencast
12f19263c9999ab9288ad47b3d27eaa8.mp4