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

Jump to bottom

DataViews: iterate on filter's API #55440

Merged
merged 3 commits into from
Oct 19, 2023
Merged

DataViews: iterate on filter's API #55440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
146981d
Allow filters to be declared as a string
oandregal Oct 18, 2023
5a99e93
Provide a default filter reset value that filters can override
oandregal Oct 18, 2023
628e2a3
Document changes
oandregal 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
16 changes: 9 additions & 7 deletions packages/edit-site/src/components/dataviews/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ The fields describe the dataset. For example:
);
},
elements: [
{ value: 1, label: 'admin' }
{ value: 2, label: 'user' }
{ value: 1, label: 'Admin' }
{ value: 2, label: 'User' }
]
filters: [
{ id: 'author', type: 'enumeration' },
{ id: 'author_search', type: 'search' }
'enumeration',
{ id: 'author_search', type: 'search', name: __( 'Search by author' ) }
],
},
]
Expand All @@ -85,10 +85,12 @@ The fields describe the dataset. For example:
- `getValue`: function that returns the value of the field.
- `render`: function that renders the field.
- `elements`: a set of valid values for the field.
- `filters`: what filters are available. A filter is an object with `id` and `type` as properties:
- `id`: unique identifier for the filter. Matches the entity query param.
- `filters`: what filters are available for the user to use. A filter contains the following properties:
- `id`: unique identifier for the filter. Matches the entity query param. If not provided, the field's `id` is used.
- `name`: nice looking name for the filter. If not provided, the field's `header` is used.
- `type`: the type of filter. One of `search` or `enumeration`.

- `resetLabel`: the label for the reset option of the filter. If none provided, `All` is used.
- `resetValue`: the value for the reset option of the filter. If none provedid, `''` is used.

## DataViews

Expand Down
39 changes: 34 additions & 5 deletions packages/edit-site/src/components/dataviews/filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,38 @@ export default function Filters( { fields, view, onChangeView } ) {
return;
}

field.filters.forEach( ( f ) => {
filters[ f.id ] = { type: f.type };
field.filters.forEach( ( filter ) => {
let id = field.id;
if ( 'string' === typeof filter ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic confuses me a bit and I think it's because of trying to introduce some short hand versions for declaring the filters. For example enumeration(for author), get's in this if block and the one below. Why not have a single way of declaring filters for now?

I might be missing something though.

Copy link
Member Author

@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.

(sorry: hit enter before the comment was ready)

We need to support the following use case: a field can have different filters. For example:

id: 'author',
filters: [
  { id: 'author', type: 'enumeration' }, // This is the IN filter.
  { id: 'author_exclude', type: 'enumeration' }, // This is the NOT IN filter.
]

However, at the moment, we don't have any NOT IN filter, so the feedback I've got is that providing a filter id that is the same as the field id is repetitive:

id: 'author',
filters: [
  { id: 'author', type: 'enumeration' }
]

That's why it's worth adding it now: most of the fields won't have multiple filters or filters whose id is different from then field id.

filters[ id ] = {
id,
name: field.header,
type: filter,
};
}

if ( 'object' === typeof filter ) {
id = filter.id || field.id;
filters[ id ] = {
id,
name: filter.name || field.header,
type: filter.type,
};
}

if ( 'enumeration' === filters[ id ]?.type ) {
const elements = [
{
value: filter.resetValue || '',
label: filter.resetLabel || __( 'All' ),
},
...( field.elements || [] ),
];
filters[ id ] = {
...filters[ id ],
elements,
};
}
} );
} );

Expand All @@ -33,7 +63,7 @@ export default function Filters( { fields, view, onChangeView } ) {
return (
<TextFilter
key={ filterName }
id={ filterName }
filter={ filter }
view={ view }
onChangeView={ onChangeView }
/>
Expand All @@ -43,8 +73,7 @@ export default function Filters( { fields, view, onChangeView } ) {
return (
<InFilter
key={ filterName }
id={ filterName }
fields={ fields }
filter={ filter }
view={ view }
onChangeView={ onChangeView }
/>
Expand Down
12 changes: 5 additions & 7 deletions packages/edit-site/src/components/dataviews/in-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,19 @@ import { unlock } from '../../lock-unlock';

const { cleanEmptyObject } = unlock( blockEditorPrivateApis );

export default ( { id, fields, view, onChangeView } ) => {
const field = fields.find( ( f ) => f.id === id );

export default ( { filter, view, onChangeView } ) => {
return (
<SelectControl
value={ view.filters[ id ] }
value={ view.filters[ filter.id ] }
prefix={
<InputControlPrefixWrapper
as="span"
className="dataviews__select-control-prefix"
>
{ field.header + ':' }
{ filter.name + ':' }
</InputControlPrefixWrapper>
}
options={ field?.elements || [] }
options={ filter.elements }
onChange={ ( value ) => {
if ( value === '' ) {
value = undefined;
Expand All @@ -38,7 +36,7 @@ export default ( { id, fields, view, onChangeView } ) => {
...currentView,
filters: cleanEmptyObject( {
...currentView.filters,
[ id ]: value,
[ filter.id ]: value,
} ),
} ) );
} }
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/dataviews/text-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { SearchControl } from '@wordpress/components';
*/
import useDebouncedInput from '../../utils/use-debounced-input';

export default function TextFilter( { id, view, onChangeView } ) {
export default function TextFilter( { filter, view, onChangeView } ) {
const [ search, setSearch, debouncedSearch ] = useDebouncedInput(
view.filters[ id ]
view.filters[ filter.id ]
);
const onChangeViewRef = useRef( onChangeView );
useEffect( () => {
Expand All @@ -24,7 +24,7 @@ export default function TextFilter( { id, view, onChangeView } ) {
page: 1,
filters: {
...currentView.filters,
[ id ]: debouncedSearch,
[ filter.id ]: debouncedSearch,
},
} ) );
}, [ debouncedSearch ] );
Expand Down
33 changes: 17 additions & 16 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 @@ -133,7 +133,9 @@ export default function PagePages() {
</VStack>
);
},
filters: [ { id: 'search', type: 'search' } ],
filters: [
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the global filter and yet we apply it in title. This should work in any field right? Maybe it's better to have flag for that and add it in the main component?

Copy link
Member Author

@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.

Yeah, search is weird. See related conversation #55270 (comment)

I'm trying to run with the idea we talked about: filters are part of the fields API. Though, the more I think about it, the more it makes sense that filters are an independent property of the DataViews component:

<DataViews
  fields={fields}
  view={view}
  filters={filters}
  actions={actions}
  ...
/>

Though, right now, I'm trying to stretch a bit the idea of filters as part of the fields API before giving up on it. One use case that will help clarify this is implementing column filters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess will see how the API will evolve. In general a global filter could be part of the API by just having an interface, consumers can abide to. So they would have to map keys(like search) with their own respective param.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I'd extract search from the fields and pass it directly as a prop, as you suggested #55475

{ id: 'search', type: 'search', name: __( 'Search' ) },
],
maxWidth: 400,
sortingFn: 'alphanumeric',
enableHiding: false,
Expand All @@ -150,27 +152,27 @@ export default function PagePages() {
</a>
);
},
filters: [ { id: 'author', type: 'enumeration' } ],
elements: [
{
value: '',
label: __( 'All' ),
},
...( authors?.map( ( { id, name } ) => ( {
filters: [ 'enumeration' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

If a filter is enumeration it requires the elements prop, right? Why we have them disconnected in the declaration?

Copy link
Member Author

@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.

The rationale is that the elements of a field will be used in a few scenarios: filters, when editing the cells, etc.

elements:
authors?.map( ( { id, name } ) => ( {
value: id,
label: name,
} ) ) || [] ),
],
} ) ) || [],
},
{
header: __( 'Status' ),
id: 'status',
getValue: ( { item } ) =>
postStatuses[ item.status ] ?? item.status,
filters: [ { type: 'enumeration', id: 'status' } ],
elements: [
{ label: __( 'All' ), value: 'publish,draft' },
...( ( postStatuses &&
filters: [
{
type: 'enumeration',
id: 'status',
resetValue: 'publish,draft',
},
],
elements:
( postStatuses &&
Object.entries( postStatuses )
.filter( ( [ slug ] ) =>
[ 'publish', 'draft' ].includes( slug )
Expand All @@ -179,8 +181,7 @@ export default function PagePages() {
value: slug,
label: name,
} ) ) ) ||
[] ),
],
[],
enableSorting: false,
},
{
Expand Down
Loading