DataViews: implement multiple selection for filters#59610
Conversation
|
Size Change: +354 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
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. |
| filterBy: { | ||
| operators: [ OPERATOR_IN ], | ||
| isPrimary: true, | ||
| singleSelection: true, |
There was a problem hiding this comment.
I feel like we should have the single selection by default, similar for example to select tag with the multiple attribute.
There was a problem hiding this comment.
I started that way, but then looked at the current uses cases:
- Pages. Both Author and Status should be multiselection.
- Patterns. Sync should be single selection.
- Templates. Author filter should be multiselection.
- Parts. Author filter should be multiselection.
All filters but one (sync in patterns) use multiselection, so I thought we should actually optimize for the common use case.
There was a problem hiding this comment.
In general I think this will probably be inferred from the new operators.
There was a problem hiding this comment.
In general I think this will probably be inferred from the new operators.
Can you expand this comment?
There was a problem hiding this comment.
Instead of the singleSelection flag we could use a flag to declare what are the supported relationship? As in:
relationship: [ ANY_OF, ALL_OF ]I like this, going to investigate a bit.
There was a problem hiding this comment.
relationship: [ ANY_OF, ALL_OF ]
We'll need something like this for category and tag filtering. E.g. It should be possible to; view posts in Category A or Category B. Or: View posts in Category A and Category B. The inverse should also be possible (posts not in Category A or B).
The UI should provide a way for users to change this operator.
There was a problem hiding this comment.
Pushed a change for this and updated the docs to explain:
Allowed operators for fields of type
enumeration:
equal: whether an item is equal to a single value.notEqual: whether an item is not equal to a single value.in: whether an item is in a list of values.notIn: whether an item is not in a list of values.By default, a field of type
enumerationsupportsinandnotInoperators — this is, it supports multiselection and negation.A filter cannot mix single-selection (
equal,notEqual) & multi-selection (in,notIn) operators. If a single-selection operator is present in the list of valid operators, the multi-selection ones will be discarded and the filter won't allow selecting more than one item.
There was a problem hiding this comment.
Examples:
- Single-selection filter:
operators: [ OPERATOR_EQUAL, OPERATOR_NOT_EQUAL ] - Multi-selection filter (the default):
operators: [ OPERATOR_IN, OPERATOR_NOT_IN ] - Single-selection filter that does not support negation:
operators: [ OPERATOR_EQUAL ] - etc.
This PR only adds the single-selection operators and makes the existing filters (in/not in) multi-selection. They are OR operators, so I'll add the AND operator in a follow-up.
For reference, in terms of nomenclature we'd have (and their negations):
| Constant | Filter definition | Label | Example |
|---|---|---|---|
| OPERATOR_EQUAL | equal |
is |
Author is Admin |
| OPERATOR_NOT_EQUAL | notEqual |
is not |
Author is not Admin |
| OPERATOR_IN | in |
in |
Author in Admin, Editor |
| OPERATOR_NOT_IN | notIn |
not in |
Author not in Admin, Editor |
| OPERATOR_ALL* | all |
is all of |
Tags is all of Books, English, School |
| OPERATOR_NOT_ALL* | notAll |
is not all of |
Tags is none of Books, English, School |
* To be implemented in a future PR, shared for reference.
|
Thanks for the PR! I haven't checked the code much yet, but tested it a bit. We should probably truncate the selected values, because if there are too many the chip gets quite big. Additionally it's not clear to me whether the selected values have an Finally by quickly testing I'd expect the templates list to work, since you have the single value as default for now, but it doesn't. Again I didn't check the code though why this is happening.. |
Do you have more than 10 templates by chance? Combobox wasn't implemented, I just did it now. |
|
Found a weird thing with the Ariakit.ComboboxProvider. Notice how clicking twice on the same element doesn't trigger the Gravacao.do.ecra.2024-03-06.as.10.26.42.mov |
@jameskoster any thoughts? I understand the rationale for limiting the space, though it comes with hiding the values that are selected — which requires the user to actually open the filter to see them all and they may or may not be contiguous. I'm on the fence about this one. |
|
I think we'll be forced to truncate at some point – a really large site could choose many values for a single filter. Perhaps we could display a badge when more than X values are chosen, or when a certain letter-count threshold is breached? 
|
50538ac to
c0a9901
Compare
I went with truncation for the time being, we can revisit later. |
|
@ntsekouras @jameskoster @jorgefilipecosta this is working as expected and all your feedback was addressed. Is there anything else to do before landing? |
|
The concern with truncating only by character count is that the user has no idea how many values are selected without opening the filter config popover. Would it be possible to truncate another way? If not the badge I suggested above then something like: If that's going to blow-up the scope of the PR then I'd be tempted to remove the truncation and explore a comprehensive solution in a follow-up. |
jorgefilipecosta
left a comment
There was a problem hiding this comment.
There is a behavior that if possible would be good to address:
At the start there is no filter applied. And all records are shown
If I select a "In" filter and then imeadtly unselect the list becomes empty. I think ideally the filter would be removed and all records would be shown.
Agreed that character truncation isn't ideal here, and that "Category is Recipes and 3 others", "Author is not Admin or 2 others", etc would be more comprehensible. Could leave two options alone though... "Status is Draft or Scheduled" for example.
Might need to watch the character count for individual values, as there's still the potential for somewhat long chips, but it's more manageable at least. |
Ah, thanks for the report. I could only reproduce in Templates & Parts, that have a custom filter mechanism. It's fixed now. |
|
I reverted the truncation until we figure out a better way. To be investigated separately. |
That's a good point. I'll make an issue for truncation. (#59696) Overall this seems very close, the only detail I'm unsure of is the operator labels – Thinking more about fields that can have multiple values (e.g. Tags/Categories), perhaps something like:
What do you think? |
74446e7 to
70e40ed
Compare
|
I've pushed the changes to adapt the code to the agreed operators: Gravacao.do.ecra.2024-03-11.as.16.43.53.movGravacao.do.ecra.2024-03-11.as.16.44.24.movGravacao.do.ecra.2024-03-11.as.16.44.09.mov |
Added this as a follow-up. |
|
#59953 adds the |
| /* translators: 1: Filter name. 3: Filter value. e.g.: "Author is any: Admin, Editor". */ | ||
| __( '<Name>%1$s is any: </Name><Value>%2$s</Value>' ), | ||
| filter.name, | ||
| activeElements.map( ( element ) => element.label ).join( ', ' ) |
There was a problem hiding this comment.
I suspect that some (few) locales will want to have a say in the punctuation used to enumerate filters:
activeElements.map( ... ).join(
/* translators: string used to join together active filters, e.g. "Author is any: Admin, Editor" */
__( ', ')
)@swissspidy: do you know if this is done elsewhere in WP?
| return createInterpolateElement( | ||
| sprintf( | ||
| /* translators: 1: Filter name. 3: Filter value. e.g.: "Author is any: Admin, Editor". */ | ||
| __( '<Name>%1$s is any: </Name><Value>%2$s</Value>' ), |
There was a problem hiding this comment.
From a UX perspective, I wonder if we shouldn't switch the string based on the number of active filters:
- Is any of: Admin, Editor
- Is: Editor
… or if that would make things more confusing because "Is:" overlaps with OPERATOR_IS. 🤔
(I also wonder whether "Is any of" would sound more natural.)
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org>
Part of #55083
Related #55100
What?
Implements the ability to select multiple items in a filter.
Multi-selection:
is any/is none, e.g.:Status is any: Draft, ScheduledSingle-selection:
is/is not, e.g.:Sync status is: Synced.Why?
We need to support both single and multi selection in filters.
How?
Filters can declare whether they allow single-selection or multi-selection by using the
filterBy.operatorsproperty:filterBy.operators: [ OPERATOR_IS, OPERATOR_IS_NOT ]a filter can configure single-selectionfilterBy.operators: [ OPERATOR_IS_ANY, OPERATOR_IS_NONE ]a filter can configure multi-selectionisisisNotis notisAnyis anyisNoneis noneMulti-selection filter (Author & Status filters in Pages):
Gravacao.do.ecra.2024-03-11.as.16.44.24.mov
Singe-selection filter (Sync filter in Patterns):
Gravacao.do.ecra.2024-03-11.as.16.44.09.mov
Testing Instructions
Verify that the filters work as described for:
TODO
OPERATOR_IN,OPERATOR_NOT_INbeing multi-selection from now on.Follow-ups
is/is not.