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

[Data views]: Refactor to allow the rendering to be generated using a fields description #55444

Closed
wants to merge 2 commits into from
+104 −49
Closed

[Data views]: Refactor to allow the rendering to be generated using a fields description #55444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c8debe
[Data views]: Refactor to allow the rendering to be generated using a…
ntsekouras Oct 18, 2023
2f354f3
remove dataConfig
ntsekouras Oct 18, 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
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/dataviews/dataviews.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function DataViews( {
const _fields = useMemo( () => {
return fields.map( ( field ) => ( {
...field,
render: field.render || field.getValue,
render: field.render?.( field.getValue ) || field.getValue,
} ) );
}, [ fields ] );
return (
Expand Down
25 changes: 25 additions & 0 deletions packages/edit-site/src/components/dataviews/field-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Internal dependencies
*/
import Media from '../../components/media';

export const coreFieldTypes = {
string: {
render:
( getValue ) =>
( { item } ) =>
getValue( { item } ),
},
date: {
render:
( getValue ) =>
( { item } ) => <time>{ getValue( { item } ) }</time>,
},
image: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works for WP images now, where we pass the attachment id. Later we could change name here and also add a field type that gets the src or more props and renders just an img tag.

render:
( getValue ) =>
( { item } ) => (
<Media id={ getValue( { item } ) } size={ [ 'thumbnail' ] } />
),
},
};
1 change: 1 addition & 0 deletions packages/edit-site/src/components/dataviews/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export { default as DataViews } from './dataviews';
export { coreFieldTypes } from './field-types';
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/dataviews/view-grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function ViewGrid( { data, fields, view, actions } ) {
);
const visibleFields = fields.filter(
( field ) =>
! view.hiddenFields.includes( field.id ) &&
! view.hiddenFields?.includes( field.id ) &&
field.id !== view.layout.mediaField
);
return (
Expand Down
123 changes: 76 additions & 47 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 @@ -16,7 +16,7 @@ import { dateI18n, getDate, getSettings } from '@wordpress/date';
*/
import Page from '../page';
import Link from '../routes/link';
import { DataViews } from '../dataviews';
import { DataViews, coreFieldTypes } from '../dataviews';
import useTrashPostAction from '../actions/trash-post';
import Media from '../media';

Expand Down Expand Up @@ -96,43 +96,64 @@ export default function PagePages() {
id: 'featured-image',
header: __( 'Featured Image' ),
getValue: ( { item } ) => item.featured_media,
render: ( { item, view: currentView } ) =>
!! item.featured_media ? (
<Media
className="edit-site-page-pages__featured-image"
id={ item.featured_media }
size={
currentView.type === 'list'
? [ 'thumbnail', 'medium', 'large', 'full' ]
: [ 'large', 'full', 'medium', 'thumbnail' ]
}
/>
) : null,
...coreFieldTypes.image,
Copy link
Member

@oandregal oandregal Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you clarify why we do this if featured-image defines its own render? (same for the other fields that end up overriding the default render)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now it doesn't do more, but the goal is to have more default things like edit. Maybe it's makes more sense to try to see how the edit would be here and evaluate again. I'll convert to draft.

Copy link
Member

Choose a reason for hiding this comment

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

👍

render:
( getValue ) =>
( { item, view: currentView } ) => {
const value = getValue( { item } );
if ( ! value ) {
return null;
}
return (
<Media
className="edit-site-page-pages__featured-image"
id={ value }
size={
currentView.type === 'list'
? [
'thumbnail',
'medium',
'large',
'full',
]
: [
'large',
'full',
'medium',
'thumbnail',
]
}
/>
);
},
enableSorting: false,
},
{
header: __( 'Title' ),
id: 'title',
getValue: ( { item } ) => item.title?.rendered || item.slug,
render: ( { item } ) => {
return (
<VStack spacing={ 1 }>
<Heading as="h3" level={ 5 }>
<Link
params={ {
postId: item.id,
postType: item.type,
canvas: 'edit',
} }
>
{ decodeEntities(
item.title?.rendered || item.slug
) || __( '(no title)' ) }
</Link>
</Heading>
</VStack>
);
},
...coreFieldTypes.string,
render:
( getValue ) =>
( { item } ) => {
return (
<VStack spacing={ 1 }>
<Heading as="h3" level={ 5 }>
<Link
params={ {
postId: item.id,
postType: item.type,
canvas: 'edit',
} }
>
{ decodeEntities(
getValue( { item } )
) || __( '(no title)' ) }
</Link>
</Heading>
</VStack>
);
},
filters: [ { id: 'search', type: 'search' } ],
maxWidth: 400,
sortingFn: 'alphanumeric',
Expand All @@ -142,14 +163,17 @@ export default function PagePages() {
header: __( 'Author' ),
id: 'author',
getValue: ( { item } ) => item._embedded?.author[ 0 ]?.name,
render: ( { item } ) => {
const author = item._embedded?.author[ 0 ];
return (
<a href={ `user-edit.php?user_id=${ author.id }` }>
{ author.name }
</a>
);
},
...coreFieldTypes.string,
render:
( getValue ) =>
( { item } ) => {
const author = item._embedded?.author[ 0 ];
return (
<a href={ `user-edit.php?user_id=${ author.id }` }>
{ getValue( { item } ) }
</a>
);
},
filters: [ { id: 'author', type: 'enumeration' } ],
elements: [
{
Expand All @@ -167,6 +191,7 @@ export default function PagePages() {
id: 'status',
getValue: ( { item } ) =>
postStatuses[ item.status ] ?? item.status,
...coreFieldTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

So far, this is the only change of this PR, correct? status will now have a render (aka cell) while it didn't before. Presumably because tanstack handled it for us, and now we handle it directly via the core's types.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, given what we do at packages/edit-site/src/components/dataviews/dataviews.js:

render: field.render?.( field.getValue ) || field.getValue,

We don't need this, do we?

filters: [ { type: 'enumeration', id: 'status' } ],
elements: [
{ label: __( 'All' ), value: 'publish,draft' },
Expand All @@ -187,13 +212,17 @@ export default function PagePages() {
header: __( 'Date' ),
id: 'date',
getValue: ( { item } ) => item.date,
render: ( { item } ) => {
const formattedDate = dateI18n(
getSettings().formats.datetimeAbbreviated,
getDate( item.date )
);
return <time>{ formattedDate }</time>;
},
...coreFieldTypes.date,
render:
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we could update the getValue to:

getValue( { item } ) =>  dateI18n(
  getSettings().formats.datetimeAbbreviated,
  getDate( item.date )
)

and then use the default render provided by the core date type.

Or the other way around: make the dateI18n part of the core's type. Does that make sense?

I guess my point is that unless we benefit from introducing the core's types, it's hard to justify?

( getValue ) =>
( { item } ) => {
const formattedDate = dateI18n(
getSettings().formats.datetimeAbbreviated,
getDate( getValue( { item } ) )
);
return <time>{ formattedDate }</time>;
},
enableSorting: false,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enableSorting: false,

},
],
[ postStatuses, authors ]
Expand Down
Loading