-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add author and status filter to the page list #55270
Changes from all commits
eb63c42
f8ec790
55e2dbb
9450732
0e0328c
9fac3a5
cd42338
fde57a7
8277b89
a23f699
b76394b
22a3d87
32e06e4
c783215
6fb27a1
4804e94
1e187f4
6b1b68f
8a85762
60accfd
ce868f6
2fd1ebc
56309e8
05df50a
b87eece
f7796e6
0756dce
0e3a71a
7a8433a
4f5b193
e339b28
f39126d
ba82810
27bcf78
9ed4bdd
61c5565
1fd75cd
280dbd2
f7b0708
14b2ab8
4bd0987
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import TextFilter from './text-filter'; | ||
import InFilter from './in-filter'; | ||
|
||
export default function Filters( { fields, view, onChangeView } ) { | ||
const filters = {}; | ||
fields.forEach( ( field ) => { | ||
if ( ! field.filters ) { | ||
return; | ||
} | ||
|
||
field.filters.forEach( ( f ) => { | ||
filters[ f.id ] = { type: f.type }; | ||
} ); | ||
} ); | ||
|
||
return ( | ||
view.visibleFilters?.map( ( filterName ) => { | ||
const filter = filters[ filterName ]; | ||
|
||
if ( ! filter ) { | ||
return null; | ||
} | ||
|
||
if ( filter.type === 'search' ) { | ||
return ( | ||
<TextFilter | ||
key={ filterName } | ||
id={ filterName } | ||
view={ view } | ||
onChangeView={ onChangeView } | ||
/> | ||
); | ||
} | ||
if ( filter.type === 'enumeration' ) { | ||
return ( | ||
<InFilter | ||
key={ filterName } | ||
id={ filterName } | ||
fields={ fields } | ||
view={ view } | ||
onChangeView={ onChangeView } | ||
/> | ||
); | ||
} | ||
|
||
return null; | ||
} ) || __( 'No filters available' ) | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
__experimentalInputControlPrefixWrapper as InputControlPrefixWrapper, | ||
SelectControl, | ||
} from '@wordpress/components'; | ||
import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { unlock } from '../../lock-unlock'; | ||
|
||
const { cleanEmptyObject } = unlock( blockEditorPrivateApis ); | ||
|
||
export default ( { id, fields, view, onChangeView } ) => { | ||
const field = fields.find( ( f ) => f.id === id ); | ||
|
||
return ( | ||
<SelectControl | ||
value={ view.filters[ id ] } | ||
prefix={ | ||
<InputControlPrefixWrapper | ||
as="span" | ||
className="dataviews__select-control-prefix" | ||
> | ||
{ field.header + ':' } | ||
</InputControlPrefixWrapper> | ||
} | ||
options={ field?.elements || [] } | ||
onChange={ ( value ) => { | ||
if ( value === '' ) { | ||
value = undefined; | ||
} | ||
|
||
onChangeView( ( currentView ) => ( { | ||
...currentView, | ||
filters: cleanEmptyObject( { | ||
...currentView.filters, | ||
[ id ]: value, | ||
} ), | ||
} ) ); | ||
} } | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,13 +32,17 @@ const defaultConfigPerViewType = { | |
export default function PagePages() { | ||
const [ view, setView ] = useState( { | ||
type: 'list', | ||
search: '', | ||
filters: { | ||
search: '', | ||
status: 'publish, draft', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for enumerations we are going to support "is" filters and "is not" filters. Should our structure take that into account and use something like:
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to implement a
I think something like that is doable for top-level filters as well. And the view would hold the data as it's passed to the endpoint: {
filters: {
search: '...',
author_exclude: '...',
author: '...'
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think at the UI level a single component will render the in and not in filter. That seems to be what happens in current applications.
Because in and notIn are a single type of filter that renders a UI that will allow to pick an author part of the set or exclude an author from the set. Rendering two components separately will easily pollute the UI. The same for text filters, if a field is of type text its filter will allow in a single component things like contains, starts with etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with your suggestion #55440 changes a bit how the "reset option" works: having one by default but allowing filters to change it, should they need. This would help to provide a different string for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}, | ||
page: 1, | ||
perPage: 5, | ||
sort: { | ||
field: 'date', | ||
direction: 'desc', | ||
}, | ||
visibleFilters: [ 'search', 'author', 'status' ], | ||
// All fields are visible by default, so it's | ||
// better to keep track of the hidden ones. | ||
hiddenFields: [ 'date', 'featured-image' ], | ||
|
@@ -63,8 +67,7 @@ export default function PagePages() { | |
_embed: 'author', | ||
order: view.sort?.direction, | ||
orderby: view.sort?.field, | ||
search: view.search, | ||
status: [ 'publish', 'draft' ], | ||
...view.filters, | ||
} ), | ||
[ view ] | ||
); | ||
|
@@ -75,6 +78,10 @@ export default function PagePages() { | |
totalPages, | ||
} = useEntityRecords( 'postType', 'page', queryArgs ); | ||
|
||
const { records: authors } = useEntityRecords( 'root', 'user', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to have this data up-front, only when the user goes to filter (or edit cells, in the future). This data could be lazy loaded upon events (opening the filter menu, etc.). Leave the comment here as food for thought to be addressed in follow-ups. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general this is a filter that we shouldn't pre load all records, not only for performance, but also about the REST API limits to 100. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment is a bit stale, since the approach has changed: the filter is now always present in the UI (before it was only visible as part of the columns' actions). Does that change your thinking? What should we do here instead? |
||
who: 'authors', | ||
} ); | ||
|
||
const paginationInfo = useMemo( | ||
() => ( { | ||
totalItems, | ||
|
@@ -126,6 +133,7 @@ export default function PagePages() { | |
</VStack> | ||
); | ||
}, | ||
filters: [ { id: 'search', type: 'search' } ], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the "search" is the same thing as "title". One is a "global search" that searches in multiple fields and not just the "title". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, I shared some thoughts in this thread. The way I see it, We may need a special treatment for filters like this one. I know tanstack differentiates between global and column filters, but I'm not sure if that's enough for us. We may need more granularity, allowing filters to declare in which context they should be rendered: top-level, table view (columns), grid view, kanban, etc. I'd like to stretch it a bit and see how far it gets us. I'll prepare a follow-up to explore rendering the filters in the columns, and that should inform the needs for this API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extracting |
||
maxWidth: 400, | ||
sortingFn: 'alphanumeric', | ||
enableHiding: false, | ||
|
@@ -142,12 +150,37 @@ export default function PagePages() { | |
</a> | ||
); | ||
}, | ||
filters: [ { id: 'author', type: 'enumeration' } ], | ||
elements: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
{ | ||
value: '', | ||
label: __( 'All' ), | ||
}, | ||
...( authors?.map( ( { id, name } ) => ( { | ||
value: id, | ||
label: name, | ||
} ) ) || [] ), | ||
], | ||
}, | ||
{ | ||
header: __( 'Status' ), | ||
id: 'status', | ||
accessorFn: ( page ) => | ||
postStatuses[ page.status ] ?? page.status, | ||
filters: [ { type: 'enumeration', id: 'status' } ], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the "id" here is a bit weird, it seems to just duplicate the field id for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #55440 addresses this. |
||
elements: [ | ||
{ label: __( 'All' ), value: 'publish,draft' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we limiting to "publish" and "draft"? There might be more statuses like "schedule"... Why not use the REST API for this. Also, why "all" has a value and not just "unset" the filter. I also believe the "all" could be added automatically and not provided in this elements config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't know. This was in the original query. If all statuses are used, we don't need to provide the "unset" value for the endpoint. I'll look into this in a follow-up.
This is addressed at #55440 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Listing pages with any status at #55476 |
||
...( ( postStatuses && | ||
Object.entries( postStatuses ) | ||
.filter( ( [ slug ] ) => | ||
[ 'publish', 'draft' ].includes( slug ) | ||
) | ||
.map( ( [ slug, name ] ) => ( { | ||
value: slug, | ||
label: name, | ||
} ) ) ) || | ||
[] ), | ||
], | ||
enableSorting: false, | ||
}, | ||
{ | ||
|
@@ -163,7 +196,7 @@ export default function PagePages() { | |
enableSorting: false, | ||
}, | ||
], | ||
[ postStatuses ] | ||
[ postStatuses, authors ] | ||
); | ||
|
||
const trashPostAction = useTrashPostAction(); | ||
|
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 like this new component