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

Add status entity and use it in "admin views" experiment #55050

Merged
merged 6 commits into from
Oct 5, 2023
Merged

Add status entity and use it in "admin views" experiment #55050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7600286
Declare entity config for statuses endpoint
oandregal Oct 4, 2023
16cd768
Make records & isLoading specific to pages
oandregal Oct 4, 2023
1347cec
Fetch statuses from entity
oandregal Oct 4, 2023
96a0feb
Adapt endpoint response (object collection) to entities format (array)
oandregal Oct 5, 2023
86c8b42
Take status labels from entity
oandregal Oct 5, 2023
1ddc450
Provide the status as fallback for when the status entity does not ha…
oandregal Oct 5, 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
9 changes: 9 additions & 0 deletions packages/core-data/src/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,15 @@ export const rootEntitiesConfig = [
baseURLParams: { context: 'edit' },
key: 'plugin',
},
{
label: __( 'Post status' ),
name: 'status',
kind: 'root',
baseURL: '/wp/v2/statuses',
baseURLParams: { context: 'edit' },
plural: 'postStatuses',
key: 'slug',
},
];

export const additionalEntityConfigLoaders = [
Expand Down
24 changes: 14 additions & 10 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 @@ -22,6 +22,7 @@ import PageActions from '../page-actions';
import { DataViews, PAGE_SIZE_VALUES } from '../dataviews';

const EMPTY_ARRAY = [];
const EMPTY_OBJECT = {};

export default function PagePages() {
const [ reset, setResetQuery ] = useState( ( v ) => ! v );
Expand All @@ -32,12 +33,14 @@ export default function PagePages() {
pageSize: PAGE_SIZE_VALUES[ 0 ],
} );
// Request post statuses to get the proper labels.
const [ postStatuses, setPostStatuses ] = useState( EMPTY_ARRAY );
useEffect( () => {
apiFetch( {
path: '/wp/v2/statuses',
} ).then( setPostStatuses );
}, [] );
const { records: statuses } = useEntityRecords( 'root', 'status' );
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 wanted to optimize the status retrieval to:

  • Only return name and slug, not all schema fields.
  • Only return the statuses we actually expect to use (publish, draft), as the page entity only requests pages with those statuses.

Though it doesn't seem the statuses endpoint supports that kind of filtering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes it's better to load the full object for performance, that way it's cached and shared with other usage of the same entity.

Not sure it applies here but yeah, this seems fine to me I think

const postStatuses =
statuses === null
? EMPTY_OBJECT
: statuses.reduce( ( acc, status ) => {
acc[ status.slug ] = status.name;
return acc;
}, EMPTY_OBJECT );
Comment on lines +37 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

@oandregal: This breaks a few things in the component — even though right now that's asymptomatic. :)

The cause is the use of EMPTY_OBJECT inside the Array#reduce call, because the reducer will mutate that initial value. This means that:

  • postStatuses will always have the exact same object reference, regardless of whether statuses === null, and thus any subsequent logic in the component that relies on reference changes (e.g. useMemo( ..., [ postStatuses ] ) will not fire correctly.
  • EMPTY_OBJECT will become non-empty as soon as statuses resolves.

Proof of bug:

const EMPTY_OBJECT = {};

function getPostStatuses( rawStatuses ) {
	return rawStatuses === null
		? EMPTY_OBJECT
		: rawStatuses.reduce( ( acc, status ) => {
				acc[ status.slug ] = status.name;
				return acc;
		  }, EMPTY_OBJECT );
}

const before = getPostStatuses( null );
const after = getPostStatuses( [ { slug: 'draft', name: 'Draft' } ] );

console.log( 'Has the reference changed?', before !== after );
console.log(
	'Is EMPTY_OBJECT empty?',
	Object.keys( EMPTY_OBJECT ).length === 0
);

The immediate fix is to not use EMPTY_OBJECT in the mutating context:

	const postStatuses =
		statuses === null
			? EMPTY_OBJECT
			: statuses.reduce( ( acc, status ) => {
					acc[ status.slug ] = status.name;
					return acc;
-			  }, EMPTY_OBJECT );
+			  }, {} );

But of course this means that postStatuses will always have a new reference even when nothing has changed, so the solution is to use useMemo. But, while we are here, I recommend against using Array#reduce in favour of clearer higher-level constructs:

	const postStatuses = useMemo(
		() =>
			statuses === null
				? EMPTY_OBJECT
				: Object.fromEntries(
						statuses.map( ( { slug, name } ) => [ slug, name ] )
				  ),
		[ statuses ]
	);

I'll turn this into a PR, but I wanted to give you my full explanation first!

Copy link
Contributor

Choose a reason for hiding this comment

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

-> #55314


// TODO: probably memo other objects passed as state(ex:https://tanstack.com/table/v8/docs/examples/react/pagination-controlled).
const pagination = useMemo(
Expand Down Expand Up @@ -66,7 +69,7 @@ export default function PagePages() {
reset,
]
);
const { records, isResolving: isLoading } = useEntityRecords(
const { records: pages, isResolving: isLoadingPages } = useEntityRecords(
'postType',
'page',
queryArgs
Expand Down Expand Up @@ -136,7 +139,8 @@ export default function PagePages() {
header: 'Status',
id: 'status',
cell: ( props ) =>
postStatuses[ props.row.original.status ]?.name,
postStatuses[ props.row.original.status ] ??
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 thought I'd also update the fallback we show when the post statuses retrieval goes wrong (statuses are unavailable or just take a while to load): instead of leaving them blank, we can use the already available status property of the page.

props.row.original.status,
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this come from?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you show the key. Fine 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to show the key here?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an advantage to pre-filling the table with the slug status over starting with an empty string: it acts as a better placeholder for the content.

By using the slug as fallback, the table already has the size of the content (because the slug for Publish is publish and for Draft is draft, etc.). This avoids "visual resizes of the table" later to adapt the column to the new content. I've uploaded some before/after videos to the description. While it's not the same issue, it can serve as an example of the visual effect resizing has when the status starts as an empty string in trunk.

},
{
header: <VisuallyHidden>{ __( 'Actions' ) }</VisuallyHidden>,
Expand All @@ -161,8 +165,8 @@ export default function PagePages() {
<Page title={ __( 'Pages' ) }>
<DataViews
paginationInfo={ paginationInfo }
data={ records || EMPTY_ARRAY }
isLoading={ isLoading }
data={ pages || EMPTY_ARRAY }
isLoading={ isLoadingPages }
fields={ fields }
options={ {
manualSorting: true,
Expand Down
Loading