Data views: Update empty/loading states #59437
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
jameskoster
added
[Type] Enhancement
jameskoster
requested review from
andrewhayward,
oandregal,
jorgefilipecosta and
a team
|
Size Change: +3 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Can't test this now, but spinner instead of text is good! |
|
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. |
t-hamano
reviewed
Feb 29, 2024
|
Interesting, I see the same thing on the Templates data view. Pages is fine. Patterns is somewhere in between – I see a flash of 'No results' before the spinner. It seems that there's a 'No results' state that appears before the loading state, perhaps that's caused by a delay while Perhaps @ntsekouras has an idea. |
|
The state in which the DataView is rendered should depend on I think it might be better to rely on After a little debugging, the status changes as shown below. You can see that
If it makes sense to use <DataViews
data={ data || EMPTY_ARRAY }
isLoading={ ! hasResolved }
view={ view }
/>In any case, this is outside the scope of this PR, so it might be okay to merge this PR first, but I think we also need to resolve this state discrepancy in DataViews🤔 cc @youknowriad I assume you are familiar with the |
ntsekouras
approved these changes
Feb 29, 2024
There was a problem hiding this comment.
LGTM, thanks!
Good catch @t-hamano! It would be good to figure out why this happens and see if it affects other usages in our code base. I'll cc @adamziel because he might have a super quick answer for this..
We can land this and follow up with at least the hasResolved usage.
It seems the isResolving is not very reliable based on this:
Returns the raw
isResolvingvalue for a given selector name,
and arguments set. May be undefined if the selector has never been resolved
or not resolved for the given set of arguments, otherwise true or false for
resolution started and completed respectively.
and I'm wondering what's the true value of it, if we can just use the hasResolved value. 🤔
|
I'm not sure which gutenberg/packages/core-data/src/hooks/use-query-select.ts Lines 112 to 115 in 941d580 It seems like the underlying gutenberg/packages/data/src/redux-store/metadata/reducer.ts Lines 51 to 57 in 941d580 gutenberg/packages/core-data/src/resolvers.js Lines 28 to 38 in 941d580 gutenberg/packages/core-data/src/actions.js Lines 35 to 41 in 941d580 My guess is the data flow goes like:
This would explain the moment when we both have data and Note that configuration could happen in another, completely valid state:
I'm not sure how |
|
The If you want to know whether the selector is currently resolving, you should call the The table of values that @t-hamano shares in #59437 (comment) is correct and expected. The values are a bit out of sync between rows 3 and 4, but that should be harmless. As @adamziel points out, it happens because the data update and resolution state update don't happen atomically, but in two separate actions. |
29 tasks
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?
SpinnerWhy?
Spinneris more consistent with loading states found elsewhereTesting Instructions
Spinnerstates.mp4
Note: It would be nice to delay the appearance of the spinner by a second or two, then fade it in. Often times the data loads quickly and the display of the spinner (or 'Loading...' text) is a bit distracting.
Note 2: We might embellish the 'No results' state to include conditional messaging. E.g. when there are no records at all include an 'Add' button. When the lack of results is due to filter config; display a prominent 'Reset filters' button.