Dataviews: list all pages, not only those with publish and draft statuses #55476
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
oandregal
requested review from
ntsekouras,
mcsf,
youknowriad,
jorgefilipecosta and
ellatrix
oandregal
added
[Type] Feature
|
Size Change: +7 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
update/dataviews-list-all-statuses
branch
from
54790cd
to
3ada739
Compare
oandregal
changed the title
ntsekouras
reviewed
Oct 19, 2023
| }; | ||
| }, [ statuses ] ); | ||
|
|
||
| useEffect( () => { |
There was a problem hiding this comment.
I feel like this is a bit overkill, at least for now. Do you feel strongly about adding it now?
There was a problem hiding this comment.
We do need to update the view upon receiving the statuses, otherwise it'd have stale data. For example: what if a site has a different set of statuses from the ones we hard-coded at DEFAULT_STATUSES?
The only thing we could remove is the DEFAULT_STATUSES to defaultStatuses check. I think it is still worth maintaining: it's just a single line of code, and it prevents an unnecessary double request.
There was a problem hiding this comment.
The only thing we could remove is the DEFAULT_STATUSES to defaultStatuses
That was what I was referring to, sorry for not clarifying. I mean it's useful to avoid the request, but the block with the comments it's big 😅 . Let's simplify this part then just a bit.
How about assign DEFAULT_STATUSES with the sorted values(so no need to split->sort->join), and also sort defaultStatuses in the useMemo. So the comparison here is just for two strings.
There was a problem hiding this comment.
Done this at 78c0609
Note that I had to add another comment:
// DEFAULT_STATUSES is intentionally sorted. Items do not have spaces in between them.
// The reason for that is to match defaultStatuses because we compare both strings below (see useEffect).
const DEFAULT_STATUSES = 'draft,future,pending,private,publish'; // All statuses but 'trash'.With the previous approach, I wanted to cover against future humans (including myself) modifying ever slightly that constant, with the consequence of breaking the comparison. Perhaps I should be more optimistic about humans :)
| postStatuses: | ||
| statuses === null | ||
| ? EMPTY_OBJECT | ||
| : Object.fromEntries( |
There was a problem hiding this comment.
statuses were handled initially like an object because with REST API, it was one. Now that it's an entity and we have an array, do we still need to all that? The difference would only be in getValue for status to Array.find.
There was a problem hiding this comment.
I don't have a strong opinion on doing it either way. Do you think we should update it in this PR?
There was a problem hiding this comment.
I think it's simplifies the code, but no strong opinions here.
There was a problem hiding this comment.
Done at 1ca610c Thanks for the nudge, it does indeed simplify the code.
|
Flaky tests detected in 3ada739679222709050ad3cd44cd1cbbe8f03794. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6571583872
|
ntsekouras
approved these changes
Oct 19, 2023
|
Feel free to address the small comments I left, or land with 🟢 CI. |
oandregal
force-pushed
the
update/dataviews-list-all-statuses
branch
from
3ada739
to
1ca610c
Compare
youknowriad
reviewed
Oct 22, 2023
| }, | ||
| } ); | ||
| } | ||
| }, [ defaultStatuses ] ); |
There was a problem hiding this comment.
There's a few things here.
- I don't think we should update the "view" programmatically. It will create an undo level...
- I don't think the initial view should depend on the REST API of the status. Can the filter be "empty" initially or fixed using a value we actually choose. If not, we shouldn't render anything and shouldn't compute the initial "view" object until the statuses are loaded (but I'd prefer to avoid this option if possible)
There was a problem hiding this comment.
The "empty" state for this filter means the endpoint will return only pages with publish status. The filter is already fixed (DEFAULT_STATUSES), and this effect will only set the view programmatically when the site has a different set of statuses from the default ones (very rare use case, though it can happen).
Does that alleviate your concerns?
There was a problem hiding this comment.
this effect will only set the view programmatically when the site has a different set of statuses from the default ones (very rare use case, though it can happen)
Not sure we should support that for the moment tbh.
There was a problem hiding this comment.
So, to confirm, you'd rather prevent the view from rendering until the statuses are loaded?
There was a problem hiding this comment.
So, to confirm, you'd rather prevent the view from rendering until the statuses are loaded?
If we really want to support the themes that change the default statuses, yes. But maybe we don't care about this for now.
There was a problem hiding this comment.
I found a way to prevent us from programatically update the view #55839
This was referenced Nov 3, 2023
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.
Part of #55083
Follow-up to #55270 (comment)
What?
Updates the pages page to retrieve all the pages (not only those with publish & draft status). Also lists all statuses in the states filter.
Why?
We want to show all status.
Testing Instructions
Gravacao.do.ecra.2023-10-19.as.09.30.36.mov