Post fields: move author from edit-site to fields package
#66939
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
youknowriad,
gigitux,
louwie17,
mcsf and
ntsekouras
|
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. |
oandregal
added
[Type] Enhancement
oandregal
commented
Nov 12, 2024
Comment on lines
+25
to
+33
| const { getEntityRecord } = select( coreStore ); | ||
| let user: User | undefined; | ||
| if ( !! item.author ) { | ||
| user = getEntityRecord( 'root', 'user', item.author ); | ||
| } | ||
| return { | ||
| imageUrl: user?.avatar_urls?.[ 48 ], | ||
| text: user?.name, | ||
| }; |
There was a problem hiding this comment.
I had to rewrite the nice-looking dynamic getUser selector into using getEntityRecord one. TypeScript was complaining because it wasn't able to find the getUser one. It was like this before:
const { getUser } = select( coreStore );
const user = getUser( item.author );
return {
imageUrl: user?.avatar_urls?.[ 48 ],
text: user?.name,
};cc @youknowriad @sirreal can we use the dynamic selectors in TypeScript? I'm fine using the general selector, but I'm curious if there's a way to use the dynamic one that I didn't find.
There was a problem hiding this comment.
In #66997 @manzoorwanijk is doing some work to improve TypeScript support.
oandregal
commented
Nov 12, 2024
| label: __( 'Author' ), | ||
| id: 'author', | ||
| type: 'integer', | ||
| ...authorField, | ||
| elements: |
There was a problem hiding this comment.
The usePostFields hook returns an isLoading that is the isLoadingAuthors. That's why I've left the elements definition in this file.
I think this approach is ok because the final goal is to expose usePostFields to consumers, so they won't have to hook the data the author field anyway.
There was a problem hiding this comment.
I think ultimately we need to think about that. Fields can have dynamic list of elements, how to represent it. I think the filter UI should be able to handle dynamic elements ...
There was a problem hiding this comment.
That's a good thought, and I don't have a good answer for that right now. A related example is the render function of this same component: it uses the core store to get the author & image name for the given item. In that sense, the data is still "loading".
Do you think we should address this differently in this PR or is good enough to land?
There was a problem hiding this comment.
Yes, I think it can be explored separately.
|
Size Change: +23 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
move/author-field
branch
from
f425bf7 to
ec9bb04
Compare
youknowriad
reviewed
Nov 13, 2024
packages/fields/src/types.ts
Outdated
| @@ -39,6 +47,8 @@ export interface BasePost extends CommonPost { | |||
| permalink_template?: string; | |||
| date?: string; | |||
| modified?: string; | |||
| author?: number; | |||
| _embedded?: Embedded; | |||
There was a problem hiding this comment.
I'm not sure about this personally. I think _embedded is not always present in BasePost unless you actually call the REST API with embed=true or something.
So my first question: can we do without this in this field? If we can, I think it's better, if we can't, maybe we should probably create a new type BasePostWithEmbeds or something that extends BasePost and use it, in that new field.
There was a problem hiding this comment.
I've pushed 23c43f7
I've seen some other issues with the data fetching for this field that should be improved (see conversation below), but I believe it should be looked at separately.
oandregal
commented
Nov 13, 2024
Comment on lines
+25
to
+29
| const { getEntityRecord } = select( coreStore ); | ||
| let user: User | undefined; | ||
| if ( !! item.author ) { | ||
| user = getEntityRecord( 'root', 'user', item.author ); | ||
| } |
There was a problem hiding this comment.
There's an opportunity for improvement in this field. This code is triggering a request for user data (avatar + username), and the Pages page may potentially trigger as many requests as pages are visible if they have different authors.
But we already have that data via multiple sources:
- The
usePostFieldshook requests all the users. - The request to the
pagesendpoint embeds the author data in the item. However, this is done is a way that is not there from the beginning, but rather the pages data come in and then later it comes the_embeddedpiece of it. This prevents us from using the_embeddeddata directly and only trigger the request if it becomes stale (the user updated the item's author via QuickEdit).
It's all a bit too convoluted, and there's no obvious solution, so I'd suggest looking at this separately from this PR.
Thoughts about potential ways forward:
- Given the item has
elementsthat are the "valid elements", we could use that as a source for data retrieval. It means augmenting the elements with miscelaneous data plus making them accessible to therenderfunction. By combining this with optimizing the data request so it is only received by DataViews when theembeddeddata is added to the entity would allow us to eliminate all author REST requests. - Finding a way to augment item data. This is related to the conversation at Data Views: Add action for pages to set site homepage #65426 (comment)
youknowriad
reviewed
Nov 14, 2024
| author?: number; | ||
| } | ||
|
|
||
| export interface BasePostWithEmbeddedAuthor extends BasePost { |
There was a problem hiding this comment.
Should this be BasePostWithEmbeddedAuthor or BasePostWithEmbeds. Basically what I'm asking is what are we doing at the request level is it something like embed=true or embed[]=author
There was a problem hiding this comment.
It's _embed=author, that's why I thought it'd be best to make it specific to that. Happy to rename it to BasePostWithEmbeds.
youknowriad
approved these changes
Nov 14, 2024
oandregal
added
[Type] Code Quality
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 #61084
What?
This PR moves the
authorfield definition fromedit-siteto thefieldspackage.Why?
In preparation for moving the whole
usePostFieldshook (conversation).Testing Instructions
Visit the Pages page and verify that the "Author" field still works as expected (filters, visualization, edit).