Post fields: move author from edit-site to fields package
#66939
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import clsx from 'clsx'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { useState } from '@wordpress/element'; | ||
| import { commentAuthorAvatar as authorIcon } from '@wordpress/icons'; | ||
| import { __experimentalHStack as HStack, Icon } from '@wordpress/components'; | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { store as coreStore } from '@wordpress/core-data'; | ||
| import type { User } from '@wordpress/core-data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { BasePostWithEmbeddedAuthor } from '../../types'; | ||
|
|
||
| function AuthorView( { item }: { item: BasePostWithEmbeddedAuthor } ) { | ||
| const { text, imageUrl } = useSelect( | ||
| ( select ) => { | ||
| const { getEntityRecord } = select( coreStore ); | ||
| let user: User | undefined; | ||
| if ( !! item.author ) { | ||
| user = getEntityRecord( 'root', 'user', item.author ); | ||
| } | ||
|
Comment on lines
+25
to
+29
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:
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:
|
||
| return { | ||
| imageUrl: user?.avatar_urls?.[ 48 ], | ||
| text: user?.name, | ||
| }; | ||
|
Comment on lines
+25
to
+33
There was a problem hiding this comment. I had to rewrite the nice-looking dynamic 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. |
||
| }, | ||
| [ item ] | ||
| ); | ||
| const [ isImageLoaded, setIsImageLoaded ] = useState( false ); | ||
| return ( | ||
| <HStack alignment="left" spacing={ 0 }> | ||
| { !! imageUrl && ( | ||
| <div | ||
| className={ clsx( 'page-templates-author-field__avatar', { | ||
| 'is-loaded': isImageLoaded, | ||
| } ) } | ||
| > | ||
| <img | ||
| onLoad={ () => setIsImageLoaded( true ) } | ||
| alt={ __( 'Author avatar' ) } | ||
| src={ imageUrl } | ||
| /> | ||
| </div> | ||
| ) } | ||
| { ! imageUrl && ( | ||
| <div className="page-templates-author-field__icon"> | ||
| <Icon icon={ authorIcon } /> | ||
| </div> | ||
| ) } | ||
| <span className="page-templates-author-field__name">{ text }</span> | ||
| </HStack> | ||
| ); | ||
| } | ||
|
|
||
| export default AuthorView; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import type { Field } from '@wordpress/dataviews'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { BasePostWithEmbeddedAuthor } from '../../types'; | ||
| import AuthorView from './author-view'; | ||
|
|
||
| const authorField: Field< BasePostWithEmbeddedAuthor > = { | ||
| label: __( 'Author' ), | ||
| id: 'author', | ||
| type: 'integer', | ||
| elements: [], | ||
| render: AuthorView, | ||
| sort: ( a, b, direction ) => { | ||
| const nameA = a._embedded?.author?.[ 0 ]?.name || ''; | ||
| const nameB = b._embedded?.author?.[ 0 ]?.name || ''; | ||
|
|
||
| return direction === 'asc' | ||
| ? nameA.localeCompare( nameB ) | ||
| : nameB.localeCompare( nameA ); | ||
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * Author field for BasePost. | ||
| */ | ||
| export default authorField; |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,15 @@ interface Links { | |
| [ key: string ]: { href: string }[] | undefined; | ||
| } | ||
|
|
||
| interface Author { | ||
| name: string; | ||
| avatar_urls: Record< string, string >; | ||
| } | ||
|
|
||
| interface EmbeddedAuthor { | ||
| author: Author[]; | ||
| } | ||
|
|
||
| export interface BasePost extends CommonPost { | ||
| comment_status?: 'open' | 'closed'; | ||
| excerpt?: string | { raw: string; rendered: string }; | ||
|
|
@@ -39,6 +48,11 @@ export interface BasePost extends CommonPost { | |
| permalink_template?: string; | ||
| date?: string; | ||
| modified?: string; | ||
| author?: number; | ||
| } | ||
|
|
||
| export interface BasePostWithEmbeddedAuthor extends BasePost { | ||
|
There was a problem hiding this comment. Should this be 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 |
||
| _embedded: EmbeddedAuthor; | ||
| } | ||
|
|
||
| export interface Template extends CommonPost { | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
usePostFieldshook returns anisLoadingthat is theisLoadingAuthors. 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
usePostFieldsto consumers, so they won't have to hook the data the author field anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought, and I don't have a good answer for that right now. A related example is the
renderfunction 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it can be explored separately.