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

DataViews: Update the data views component to pass an view object #55154

Merged
merged 7 commits into from
Oct 9, 2023
Merged

DataViews: Update the data views component to pass an view object #55154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
65ea7a8
DataViews: Update the data views component to pass an view object
youknowriad Oct 9, 2023
7854081
Remove useless config
youknowriad Oct 9, 2023
b582669
Rename the table view to list view
youknowriad Oct 9, 2023
8c9c570
Remove useless dependency
youknowriad Oct 9, 2023
ebed177
Rename column to field
youknowriad Oct 9, 2023
965ea1c
Support no sorting
youknowriad Oct 9, 2023
77e9f7b
fix sorting
ntsekouras Oct 9, 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
67 changes: 65 additions & 2 deletions 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 @@ -28,18 +28,81 @@ import TextFilter from './text-filter';
export default function DataViews( {
data,
fields,
view,
onChangeView,
isLoading,
paginationInfo,
options,
options: { pageCount },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal is to actually eliminate this prop and the paginationInfo one. The remaining information should probably be part of "data" but I'll think about these separately, the current PR just focuses on the "view config".

} ) {
const dataView = useReactTable( {
data,
columns: fields,
...options,
manualSorting: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

For now we can add these internally(manualSorting and the rest manual props), but if we feed the component with static data, these should not be set to true values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'd like for these to remain internal implementation detail, we may have a way to check whether the data is static or not depending on the "data" props we provide later.

manualFiltering: true,
manualPagination: true,
enableRowSelection: true,
state: {
sorting: view.sort
? [
{
id: view.sort.field,
desc: view.sort.direction === 'desc',
},
]
: [],
globalFilter: view.search,
pagination: {
pageIndex: view.page,
pageSize: view.perPage,
},
},
onSortingChange: ( sortingUpdater ) => {
onChangeView( ( currentView ) => {
const sort =
typeof sortingUpdater === 'function'
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
? sortingUpdater(
currentView.sort
? [
{
id: currentView.sort.field,
desc:
currentView.sort
.direction === 'desc',
},
]
: []
)
: sortingUpdater;
if ( ! sort.length ) {
return {
...currentView,
sort: {},
};
}
const [ { id, desc } ] = sort;
return {
...currentView,
sort: { field: id, direction: desc ? 'desc' : 'asc' },
};
} );
},
onGlobalFilterChange: ( value ) => {
onChangeView( { ...view, search: value, page: 0 } );
},
onPaginationChange: ( paginationUpdater ) => {
onChangeView( ( currentView ) => {
const { pageIndex, pageSize } = paginationUpdater( {
pageIndex: currentView.page,
pageSize: currentView.perPage,
} );
return { ...view, page: pageIndex, perPage: pageSize };
} );
},
getCoreRowModel: getCoreRowModel(),
getFilteredRowModel: getFilteredRowModel(),
getSortedRowModel: getSortedRowModel(),
getPaginationRowModel: getPaginationRowModel(),
pageCount,
} );
return (
<div className="dataviews-wrapper">
Expand Down
76 changes: 24 additions & 52 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 @@ -19,19 +19,23 @@ import { useState, useEffect, useMemo } from '@wordpress/element';
import Page from '../page';
import Link from '../routes/link';
import PageActions from '../page-actions';
import { DataViews, PAGE_SIZE_VALUES } from '../dataviews';
import { DataViews } from '../dataviews';

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

export default function PagePages() {
const [ reset, setResetQuery ] = useState( ( v ) => ! v );
const [ globalFilter, setGlobalFilter ] = useState( '' );
const [ paginationInfo, setPaginationInfo ] = useState();
const [ { pageIndex, pageSize }, setPagination ] = useState( {
pageIndex: 0,
pageSize: PAGE_SIZE_VALUES[ 0 ],
const [ view, setView ] = useState( {
type: 'list',
search: '',
page: 0,
perPage: 5,
sort: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately, we may want to support an array here but let's keep the existing behavior for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to rename column->field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about #55205 ?

field: 'date',
direction: 'desc',
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the visible columns is an internal property, I think we may want to add this to this object later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Fields visibility should be tied to a view and in the future also probably handle some more meta like visibility in mobile.

Copy link
Member

Choose a reason for hiding this comment

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

There are also some field metadata related to UI that could also be part of the view. For example, we currently set maxWidth via the fields (and there could be other widths according to the only docs I could find).

I'm thinking of a situation where the views are customized per user/role/etc. or even updated real-time in collaborative editing. In that scenario, the width of the columns seems like an important UI detail to maintain in the view entity – so it can be updated/synchronized/etc.

} );
const [ paginationInfo, setPaginationInfo ] = useState();
// Request post statuses to get the proper labels.
const { records: statuses } = useEntityRecords( 'root', 'status' );
const postStatuses =
Expand All @@ -42,32 +46,17 @@ export default function PagePages() {
return acc;
}, EMPTY_OBJECT );

// TODO: probably memo other objects passed as state(ex:https://tanstack.com/table/v8/docs/examples/react/pagination-controlled).
const pagination = useMemo(
() => ( { pageIndex, pageSize } ),
[ pageIndex, pageSize ]
);
const [ sorting, setSorting ] = useState( [
{ order: 'desc', orderby: 'date' },
] );
const queryArgs = useMemo(
() => ( {
per_page: pageSize,
page: pageIndex + 1, // tanstack starts from zero.
per_page: view.perPage,
page: view.page + 1, // tanstack starts from zero.
_embed: 'author',
order: sorting[ 0 ]?.desc ? 'desc' : 'asc',
orderby: sorting[ 0 ]?.id,
search: globalFilter,
order: view.sort.direction,
orderby: view.sort.field,
search: view.search,
status: [ 'publish', 'draft' ],
} ),
[
globalFilter,
sorting[ 0 ]?.id,
sorting[ 0 ]?.desc,
pageSize,
pageIndex,
reset,
]
[ view ]
);
const { records: pages, isResolving: isLoadingPages } = useEntityRecords(
'postType',
Expand All @@ -84,6 +73,9 @@ export default function PagePages() {
method: 'HEAD',
parse: false,
} ).then( ( res ) => {
// TODO: store this in core-data reducer and
Copy link

Choose a reason for hiding this comment

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

@youknowriad is there an open issue for this to follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, no issue at the moment, I was thinking about adding a todo list item to the data views overview issue

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 should be addressed in #55164

// make sure it's returned as part of useEntityRecords
// (to avoid double requests).
const totalPages = parseInt( res.headers.get( 'X-WP-TotalPages' ) );
const totalItems = parseInt( res.headers.get( 'X-WP-Total' ) );
setPaginationInfo( {
Expand All @@ -92,7 +84,7 @@ export default function PagePages() {
} );
} );
// Status should not make extra request if already did..
}, [ globalFilter, pageSize, reset ] );
}, [ queryArgs ] );

const fields = useMemo(
() => [
Expand Down Expand Up @@ -147,12 +139,7 @@ export default function PagePages() {
id: 'actions',
cell: ( props ) => {
const page = props.row.original;
return (
<PageActions
postId={ page.id }
onRemove={ () => setResetQuery() }
/>
);
return <PageActions postId={ page.id } />;
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
},
enableHiding: false,
},
Expand All @@ -168,25 +155,10 @@ export default function PagePages() {
data={ pages || EMPTY_ARRAY }
isLoading={ isLoadingPages }
fields={ fields }
view={ view }
onChangeView={ setView }
options={ {
manualSorting: true,
manualFiltering: true,
manualPagination: true,
enableRowSelection: true,
state: {
sorting,
globalFilter,
pagination,
},
pageCount: paginationInfo?.totalPages,
onSortingChange: setSorting,
onGlobalFilterChange: ( value ) => {
setGlobalFilter( value );
setPagination( { pageIndex: 0, pageSize } );
},
// TODO: check these callbacks and maybe reset the query when needed...
onPaginationChange: setPagination,
meta: { resetQuery: setResetQuery },
} }
/>
</Page>
Expand Down
Loading