DataViews: add primary filter API #58427
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
oandregal
added
[Type] Enhancement
|
Size Change: +54 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
oandregal
requested review from
ntsekouras,
jameskoster,
annezazu and
jorgefilipecosta
ntsekouras
reviewed
Jan 30, 2024
oandregal
commented
Jan 30, 2024
| @@ -332,6 +332,7 @@ export default function PagePages() { | |||
| enableSorting: false, | |||
| filterBy: { | |||
| operators: [ OPERATOR_IN ], | |||
| isPrimary: true, | |||
There was a problem hiding this comment.
This needs to be removed before merging the PR. It's set for demo purposes (simulate a situation where there is primary and secondary, so we can holistically review how the API works).
There was a problem hiding this comment.
This wasn't removed before merging the PR: #58682
oandregal
force-pushed
the
add/primary-filter-api-dataviews
branch
from
31cf33d
to
5dddf2b
Compare
ntsekouras
reviewed
Jan 30, 2024
ntsekouras
reviewed
Jan 30, 2024
| @@ -325,6 +324,7 @@ export default function DataviewsPatterns() { | |||
| elements: SYNC_FILTERS, | |||
| filterBy: { | |||
| operators: [ OPERATOR_IN ], | |||
| isPrimary: true, | |||
There was a problem hiding this comment.
Should this be inside filterBy? I think it's more of top level filters related prop.
There was a problem hiding this comment.
I did consider having it top-level, and it looked like this:
{
header: 'Sync',
id: 'sync',
isPrimaryFilter: true,
}It's weird that a field says "I'm a primary filter".
Additionally, fields only have filterBy prop related to filters. It's also the reason filterBy is an object, so we can add more props as we evolve the filters API. I prefer to group it together, to be honest. If the filterBy name is confusing, I'd be open to find alternatives such as filters, etc. in follow-ups.
There was a problem hiding this comment.
Let's probably rename then in a follow up(? 🤔 ).
ntsekouras
reviewed
Jan 30, 2024
mcsf
reviewed
Jan 30, 2024
| @@ -325,6 +324,7 @@ export default function DataviewsPatterns() { | |||
| elements: SYNC_FILTERS, | |||
| filterBy: { | |||
| operators: [ OPERATOR_IN ], | |||
| isPrimary: true, | |||
I don't think there needs to be any visual distinction in the UI element. When the new filter UX design is implemented there will be a subtle difference in the configuration popover;
This seems fine for now. We may want to allow users to re-order filters, but that's probably something to tackle when we add Custom views. |
|
So, if the state of this PR seems sensible, we could land it in time for 6.5. The only interaction I'd like to clarify is this whether the reset button should be visible when there is only one primary filter: #58427 (comment) |
|
I don't think the number of primary filters makes any difference to the logic around whether the Reset button is visible. |
Ok. Pushed this change and the reset filter is always visible, when there is one filter. This is patterns: 
|
|
Is there anything else to address here, or are we happy to land this for 6.5? ✅ |
|
That seems good, and is most aligned with where we'll end up when the new filter design is implemented. Two last details I'm not sure of;
reset.mp4 |
oandregal
force-pushed
the
add/primary-filter-api-dataviews
branch
from
10c3b65
to
b389b42
Compare
|
Updated it to work as you mentioned. I understand "primary filter" as "primary action": it's not part of the filters/actions list but always visible, but I also don't have strong opinions against doing it differently for filters. Patterns Gravacao.do.ecra.2024-01-31.as.09.38.49.movPages Gravacao.do.ecra.2024-01-31.as.09.37.38.mov |
jameskoster
approved these changes
Jan 31, 2024
ntsekouras
approved these changes
Feb 1, 2024
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
Alternative to #58367
What?
This PR implements the primary filter API. Primary filters are always visible and not listed in the "Add filter" component unless the layout is the list view.
Patterns: a single primary filter.
Gravacao.do.ecra.2024-01-31.as.09.38.49.mov
Pages: primary and secondary filters (to remove before merging: it's only here to see how it works holistically.):
Gravacao.do.ecra.2024-01-31.as.09.37.38.mov
Why?
This would allow us to have filters that are permanently visible.
How?
isPrimarytofield.filterByAPI that consumers can use.Testing Instructions
Patterns:
Pages:
Questions
See answers.
How users can tell apart primary and secondary filters when they are visible? Do we need to tell them apart?How primary & secondary filters are sorted when they are visible? This PR keeps the order as it was. It's the consumer who controls the order (first declared, first displayed). They are listed in the same order in the AddComponent and in the summaries.