Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dataviews: list all pages, not only those with publish and draft statuses #55476

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Dataviews: list all pages, not only those with publish and draft statuses #55476

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7fcbd1f
List all statuses in the filter
oandregal Oct 18, 2023
3d3d6ff
Centralize statuses in a single variable
oandregal Oct 18, 2023
8c9ce94
Set allStatuses from entity
oandregal Oct 18, 2023
afee790
allStatuses to defaultStatuses
oandregal Oct 18, 2023
0a4dc16
Make sure only 1 request is dispatched to the pages endpoint
oandregal Oct 18, 2023
c605d1c
Add comment about logic
oandregal Oct 18, 2023
882bef0
Use defaultStatuses everywhere
oandregal Oct 19, 2023
1ca610c
Simplify: remove postStatuses
oandregal Oct 19, 2023
78c0609
Simplify by presorting
oandregal Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 45 additions & 26 deletions packages/edit-site/src/components/page-pages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { __ } from '@wordpress/i18n';
import { useEntityRecords } from '@wordpress/core-data';
import { decodeEntities } from '@wordpress/html-entities';
import { useState, useMemo, useCallback } from '@wordpress/element';
import { useState, useMemo, useCallback, useEffect } from '@wordpress/element';
import { dateI18n, getDate, getSettings } from '@wordpress/date';

/**
Expand All @@ -21,7 +21,6 @@ import useTrashPostAction from '../actions/trash-post';
import Media from '../media';

const EMPTY_ARRAY = [];
const EMPTY_OBJECT = {};
const defaultConfigPerViewType = {
list: {},
grid: {
Expand All @@ -30,11 +29,14 @@ const defaultConfigPerViewType = {
};

export default function PagePages() {
// 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'.
const [ view, setView ] = useState( {
type: 'list',
filters: {
search: '',
status: 'publish, draft',
status: DEFAULT_STATUSES,
},
page: 1,
perPage: 5,
Expand All @@ -48,17 +50,39 @@ export default function PagePages() {
hiddenFields: [ 'date', 'featured-image' ],
layout: {},
} );
// Request post statuses to get the proper labels.

const { records: statuses } = useEntityRecords( 'root', 'status' );
const postStatuses = useMemo(
() =>
statuses === null
? EMPTY_OBJECT
: Object.fromEntries(
statuses.map( ( { slug, name } ) => [ slug, name ] )
),
[ statuses ]
);
const defaultStatuses = useMemo( () => {
return statuses === null
? DEFAULT_STATUSES
: statuses
.filter( ( { slug } ) => slug !== 'trash' )
.map( ( { slug } ) => slug )
.sort()
.join();
}, [ statuses ] );

useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is a bit overkill, at least for now. Do you feel strongly about adding it now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

// Only update the view if the statuses received from the endpoint
// are different from the DEFAULT_STATUSES provided initially.
//
// The pages endpoint depends on the status endpoint via the status filter.
// Initially, this code filters the pages request by DEFAULT_STATUTES,
// instead of using the default (publish).
// https://developer.wordpress.org/rest-api/reference/pages/#list-pages
//
// By doing so, it avoids a second request to the pages endpoint
// upon receiving the statuses when they are the same (most common scenario).
if ( DEFAULT_STATUSES !== defaultStatuses ) {
setView( {
...view,
filters: {
...view.filters,
status: defaultStatuses,
},
} );
}
}, [ defaultStatuses ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to confirm, you'd rather prevent the view from rendering until the statuses are loaded?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to prevent us from programatically update the view #55839


const queryArgs = useMemo(
() => ( {
Expand Down Expand Up @@ -163,25 +187,20 @@ export default function PagePages() {
header: __( 'Status' ),
id: 'status',
getValue: ( { item } ) =>
postStatuses[ item.status ] ?? item.status,
statuses?.find( ( { slug } ) => slug === item.status )
?.name ?? item.status,
filters: [
{
type: 'enumeration',
id: 'status',
resetValue: 'publish,draft',
resetValue: defaultStatuses,
},
],
elements:
( postStatuses &&
Object.entries( postStatuses )
.filter( ( [ slug ] ) =>
[ 'publish', 'draft' ].includes( slug )
)
.map( ( [ slug, name ] ) => ( {
value: slug,
label: name,
} ) ) ) ||
[],
statuses?.map( ( { slug, name } ) => ( {
value: slug,
label: name,
} ) ) || [],
enableSorting: false,
},
{
Expand All @@ -197,7 +216,7 @@ export default function PagePages() {
},
},
],
[ postStatuses, authors ]
[ statuses, authors ]
);

const trashPostAction = useTrashPostAction();
Expand Down
Loading