DataViews: iterate on filter's API #55440
Merged
Conversation
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
|
Size Change: +72 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
oandregal
added
the
[Feature] DataViews
oandregal
requested review from
mcsf,
ellatrix,
jorgefilipecosta and
ntsekouras
oandregal
added
the
[Type] Code Quality
ntsekouras
reviewed
Oct 18, 2023
| @@ -133,7 +133,9 @@ export default function PagePages() { | |||
| </VStack> | |||
| ); | |||
| }, | |||
| filters: [ { id: 'search', type: 'search' } ], | |||
| filters: [ | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I thought I'd extract search from the fields and pass it directly as a prop, as you suggested #55475
ntsekouras
reviewed
Oct 18, 2023
| label: __( 'All' ), | ||
| }, | ||
| ...( authors?.map( ( { id, name } ) => ( { | ||
| filters: [ 'enumeration' ], |
There was a problem hiding this comment.
If a filter is enumeration it requires the elements prop, right? Why we have them disconnected in the declaration?
There was a problem hiding this comment.
The rationale is that the elements of a field will be used in a few scenarios: filters, when editing the cells, etc.
ntsekouras
reviewed
Oct 18, 2023
| filters[ f.id ] = { type: f.type }; | ||
| field.filters.forEach( ( filter ) => { | ||
| let id = field.id; | ||
| if ( 'string' === typeof filter ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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.
ntsekouras
reviewed
Oct 18, 2023
There was a problem hiding this comment.
Thank you @oandregal!
Not in this PR, but since it's filters related, we could also add the has_published_posts flag when getting the users.
@ntsekouras nice one. I've prepared #55455 |
oandregal
changed the title
ntsekouras
approved these changes
Oct 19, 2023
Good feedback! What I'm thinking is that the same field can have many types of filters. That's why |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #55083
Follow-up to #55270
What?
Introduces a few changes to the filters:
Before: the
field.elementsproperty held all values, including the "reset value". After: thefield.elementsproperty only holds the set of elements and the filter provides the "reset value" by default. The filter can also change it.Testing Instructions
Verify that the filters in the "Site editor > Pages > Manage all pages" experiment work as before.