Add status entity and use it in "admin views" experiment
#55050
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
oandregal
added
[Status] In Progress
|
Size Change: +55 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
add/entity-statuses
branch
from
a6e9255 to
04a2e64
Compare
oandregal
force-pushed
the
add/entity-statuses
branch
from
04a2e64 to
1347cec
Compare
The statuses endpoint returns a collection as object, being the keys the corresponding slug of the status. However, the entities expect to work with arrays. Providing the key to the entity config they'll be automatically adapted from the object collection to an array.
oandregal
commented
Oct 5, 2023
| path: '/wp/v2/statuses', | ||
| } ).then( setPostStatuses ); | ||
| }, [] ); | ||
| const { records: statuses } = useEntityRecords( 'root', 'status' ); |
There was a problem hiding this comment.
I wanted to optimize the status retrieval to:
- Only return
nameandslug, 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.
There was a problem hiding this comment.
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
oandregal
commented
Oct 5, 2023
| @@ -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 ] ?? | |||
There was a problem hiding this comment.
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.
oandregal
changed the title
statuses entitystatus entity and use it in "admin views" experiment
youknowriad
reviewed
Oct 5, 2023
| @@ -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 ] ?? | |||
| props.row.original.status, | |||
There was a problem hiding this comment.
Where does this come from?
There was a problem hiding this comment.
oh you show the key. Fine 👍
There was a problem hiding this comment.
Do we need to show the key here?
There was a problem hiding this comment.
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.
youknowriad
approved these changes
Oct 5, 2023
There was a problem hiding this comment.
In trunk, if you navigate away and pack to the page, you'll notice that the status take some time to show up. This PR fixes that 👍
That said, the loading of this screen need special care. For instance right now, the pagination is visible while the table is still hidden and the total switches from 0 to the actual number while loading. I think we should have a dedicated todo list item about the loading stage on this page.
annezazu
added
the
[Feature] DataViews
mcsf
reviewed
Oct 12, 2023
Comment on lines
+37
to
+43
| const postStatuses = | ||
| statuses === null | ||
| ? EMPTY_OBJECT | ||
| : statuses.reduce( ( acc, status ) => { | ||
| acc[ status.slug ] = status.name; | ||
| return acc; | ||
| }, EMPTY_OBJECT ); |
There was a problem hiding this comment.
@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:
postStatuseswill always have the exact same object reference, regardless of whetherstatuses === null, and thus any subsequent logic in the component that relies on reference changes (e.g.useMemo( ..., [ postStatuses ] )will not fire correctly.EMPTY_OBJECTwill become non-empty as soon asstatusesresolves.
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!
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 #54966
What?
Creates a new entity for the
statusesendpoint and updates the pages list to using it.Why?
https://github.com/WordPress/gutenberg/pull/54966/files#r1342580462 By using the core-data package, we leverage some basic behaviors such as caching. Note the before/after for the page status when navigating away and back to the page:
Before (trunk). The status is blank until the fetching is resolved:
Gravacao.do.ecra.2023-10-05.as.14.30.36.mov
After (this PR). The status is immediately shown, as a result of being available in the core store:
Gravacao.do.ecra.2023-10-05.as.14.32.37.mov
How?
statusentity.Testing Instructions