DataViews: introduce locked view filters #65264
Closed
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
oandregal
requested review from
youknowriad,
jameskoster,
jasmussen,
jorgefilipecosta and
ntsekouras
|
cc @Souptik2001 for awareness. |
oandregal
added
[Type] Enhancement
This was referenced Sep 12, 2024
|
Size Change: +33 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
I may have limited availability to land this next week if it doesn't land earlier. In the interest of making it to 6.7, anyone should feel free to push directly to this PR. |
|
I'm not sure how can I trigger proper credits to @ @Souptik2001 as the author of the first attempt to improve this. We need to add this bit as well: |
oandregal
commented
Sep 12, 2024
| /** | ||
| * Whether the filter can be edited or removed. | ||
| */ | ||
| isLocked?: boolean; |
There was a problem hiding this comment.
I considered making this part of the fields.filterBy config, as isPrimary is. I decided against because having it as a view filter property collocates both concerns together: declaring the values & declaring those values are locked.
This made me think that fields.filterBy.isPrimary is actually not collocated properly. It should be part of the View API, not of Field API: it is about what's visible or not in the UI, not about expressing what a field can do.
There was a problem hiding this comment.
Thoughts for a potential follow-up PR to absorb primary filters as part of the View API. We could:
- Introduce
isVisibleproperty for view filters. This already exists internally. A primary filter is one that setsisVisibleto true. isVisiblefilters are always part of the view config. So, when they don't have any values, they're stored as:
const view = {
filters: [
{ field: '...', value: null, isVisible: true }
]
};
There was a problem hiding this comment.
This makes sense to me and the follow up you're suggesting.
|
Can you help me understand a bit. I'm not sure I understand why we want to pass these "filters" to the data views component if we want to hide them completely? |
|
To clarify my thoughts, if we want filters just to filter the query but we don't want them to show for the user. We can just do: const [ view, setView ] = useState({});
const filteredView = useMemo(() => {
return { ...view, filters: { ...staticFilters, ...view.filters } };
}, [view, staticFilters]);
const data = useDataFromView( filteredView );
return <DataViews data={data} view={ view } onChange={ setView } />; |
ntsekouras
reviewed
Sep 12, 2024
| filters: [], | ||
| filters: | ||
| view.filters?.filter( | ||
| ( filter ) => filter.isLocked === true |
There was a problem hiding this comment.
Nit: do we need the extra === true?
@youknowriad what do you mean here? Filters are 'created' based on the fields provided in DataViews component. This PR doesn't change that. |
👍 Yes, that would work. Adding Implementing the same behavior for Pages without Additionally, and perhaps more importantly, |
|
I don't understand why would I pass a "filter" to a DataViews component if the DataViews component doesn't need to use that field or show it anywhere. IMO, this API is not needed. |
|
I've discussed this a bit with Nik and I think there's a small confusion that we should clear. Right now the UI of DataViews doesn't allow you to add multiple filters for the same field (although it's possible with code). It's important to understand that this is just a temporary measure. We should be able to say "title starts with an and finishes with z" (so two or multiple filters using the same field). This means that a "isLocked" or "isHidden" API on the filter object won't allow the "field" to be hidden from the list of available fields to add as a filter. This is just a consequence of a temporary limitation. |
I see. There's a few options to implement multiple filters and we'd need to consider hierarchy. One that I think is promising would be: const view = {
filters: [ {
field: 'fid',
operator: 'and',
children: [
{ value: '...', operator: 'in' },
{ value: '...', operator: 'startsWith' }
],
isLocked: true,
} ]
};In this scenario, we could still lock all the filters of a field easily (see What I think is most valuable about this approach is that "custom views" are able to filter a dataset without requiring the user to provide any code. This is not what we need to solve right now, so it can wait. Perhaps in the future we may explore alternatives that filter the dataset but are unrelated to the UI. For example: const view = {
filters: [ { /* control UI */ } ],
data: {
filters: [ /* do not show up in UI: it's a pre-filter, so to speak */ ]
}
};Or perhaps we just need a different entity to manipulate/wrangle the data property from a given source ( I'll close this for now. |
|
Prepared an alternative at #65295 |
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.
Alternative to #62157
Closes #60472
Partially addresses #60468
What?
Enables consumers of DataViews to declare view filters that are locked: they cannot be edited or removed by users — and they are also hidden.
Why?
There are situations in which the view 1) wants to communicate a filter state so consumers can filter the datasource but 2) don't want that filter to be exposed to users.
How?
Introduces the
isLockedprop to view filters: a filter cannot be edited or removed by the user when it'strue. The filter is also not visible to the user anywhere.Testing Instructions
Props
Add props to @Souptik2001 for the original PR: