DataViews: pass search filter as global, unattached from fields
#55475
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
oandregal
requested review from
ntsekouras,
youknowriad,
mcsf,
ellatrix and
jorgefilipecosta
oandregal
added
[Type] Code Quality
|
Size Change: +43 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 99ed653. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6571642636
|
oandregal
force-pushed
the
update/filter-search-as-global
branch
from
079900c
to
99ed653
Compare
ntsekouras
reviewed
Oct 19, 2023
| @@ -200,6 +197,8 @@ export default function PagePages() { | |||
| [ postStatuses, authors ] | |||
| ); | |||
|
|
|||
| const filters = useMemo( () => [ { id: 'search', type: 'search' } ] ); | |||
There was a problem hiding this comment.
I'm not sure we're expecting more global filters like that, do we? The only piece of info needed with the current architecture is the key to be used in the query, and I believe we can safely start with a TextFilter outside of Filters component(render in dataviews.js).
There was a problem hiding this comment.
I'd prefer not to make it a special case, to be honest. This way, DataViews is already future-proof for any other page/endpoint/etc.
There was a problem hiding this comment.
Though it is a special case, no? It's a global filter. How it could affect any other page etc..?
There was a problem hiding this comment.
My point is that I prefer to conceptualize it as a filter and do this:
<DataViews
filters={ filters }
/>rather than making it a special case and do this:
<DataViews
search={search}
/>The reason is this: what if a different page/endpoint has a global filter that is an enumeration or any other type of filter?
The first approach is future-proof: it requires no changes in the future, it already supports any type of filter (search, enumeration, and whatever we add in the future). The second approach would require us to make changes in DataViews API (new properties, etc.) even when it already supports those other filter types.
There was a problem hiding this comment.
No strong opinions here, but I still don't get it. We still add the filters prop to the API and we're trying to support a use case that we don't have right now. The concept of having a text global filter in a list or in a collection in general is the most common one, by far I think. Maybe some others can chime in, but it's not something that important for now IMO. In my mind the prop would be something like globalFilter="$key".
There was a problem hiding this comment.
Maybe some others can chime in, but it's not something that important for now IMO. In my mind the prop would be something like globalFilter="$key".
What you are suggesting would be (note we still have to pass the label as well):
<DataViews
globalFilter={ {id: "key", label: "Label" } }
/>And what this PR proposes is:
<DataViews
filters=[
{ id: "key", label: "Label", type: "type" }
]
/>It's not that different :)
The only differences I see are:
| Alternative | Existing | |
|---|---|---|
| Property name | globalFilter |
filters |
| Property type | object | array of objects |
| Shape of objects | Only id and label |
The same as field filters. |
I feel strongly about the shape of filters being identical between field filters and global filters: I don't think we should do anything differently.
If it helps unlock this review and merge the PR, I am open to use globalFilter as name and use an object instead of an array. We could change that later if needed be.
There was a problem hiding this comment.
After thinking more about it, while I still believe the global filter is a special case and will be almost a text filter every time, passing filters array allows a consumer to have more custom filters(if they need to) that are outside the specifics of a field. Let's land this.
|
Not related to this PR, but I notice |
Ah, good catch. I've noticed something else. Let me push a few changes. |
|
Fixed the label at 781446e The other thing I was looking at was a false alarm – everything is fine. |
ntsekouras
approved these changes
Oct 19, 2023
youknowriad
reviewed
Oct 22, 2023
| @@ -228,6 +229,7 @@ export default function PagePages() { | |||
| <DataViews | |||
| paginationInfo={ paginationInfo } | |||
| fields={ fields } | |||
| filters={ filters } | |||
There was a problem hiding this comment.
I don't understand this prop? What is it for?
youknowriad
reviewed
Oct 22, 2023
| @@ -200,6 +197,10 @@ export default function PagePages() { | |||
| [ postStatuses, authors ] | |||
| ); | |||
|
|
|||
| const filters = useMemo( () => [ | |||
There was a problem hiding this comment.
To be honest, I'm not sure I'd consider "search" a filter at all. It may be its own thing.
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 (comment) and #55440 (comment)
What?
Extracts the
searchfilter from the fields definition and pass it directly to the DataViews component.Why?
The
searchfilter is a cross-field filter and doesn't benefit from any field information. It's clearer if we make that obvious by passing it directly to theDataViewscomponent. See the following conversations #55270 (comment) and #55440 (comment) as well.How?
Pass filters as a new property of the DataViews component. These filters and the ones bound to the fields are treated the same.
Testing Instructions
searchfilter and verify that it works properly.