DataViews: add support for NOT IN operator in filter
#56479
Conversation
oandregal
added
[Type] Experimental
oandregal
requested review from
ntsekouras,
youknowriad,
jameskoster,
SaxonF and
jorgefilipecosta
|
Size Change: +641 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Design wise should we do something more like notion where you don't select a "value" from the "add filter" dropdown, instead, you just select the field, and then you get a modal/popover where you can do several things:
I think this modal will scale well to all operators and type of fields... Some fields for instance might not have a fixed list of values and you want to type something instead. Screen.Recording.2023-11-23.at.3.06.54.PM.mov |
|
@youknowriad I think that might go beyond the scope of what's capable in the new DropdownMenu component and require the use of an actual modal. Cc @ciampo for confirmation. That's not to say we should do it, but it would require a little design exploration. If we wanted to use DropdownMenu something like this might work (with finessed wording)? 
|
|
I think the fact that we started with filters that consist of just item selection is maybe leading us to a direction that doesn't accommodate all the needs. We're trying to force some semantics to the dropdown menu that are not well suited for it. We want a form for each filter basically and a form can be shown in a dropdown or a modal, both work but not a dropdown menu. |
|
That's a fair point, but it is quite well optimised for the majority of use cases – it would be a shame to make those feel more complicated unnecessarily. Another option would be to not include operator options in these basic filters, and offer those options in a dedicated "Advanced filter" UI instead. Notion offers something similar: 
This is especially comprehensive as it even allows operations between filters. |
Using nested menus with radio options could work |
|
I've been working on refactoring a bit the existing components to be able to add this feature all at once in all of them. I'd like to land #56514 before this PR. |
|
I made a quick mockup for the flow Riad proposed, where the filter UIs are forms in popovers rather than DropdownMenus. 
The main difference in design is that we lose the ability to add filters as part of the flyout, and that since the new UI is a form we should probably use form elements (checkboxes and radios). I think both concessions are okay given the benefits – more flexibility in terms of what we can place in the popovers. There's a question about where to put focus when adding a new filter, I would assume on the first element in the popover but cc @andrewhayward for any thoughts on that. Edit: Also worth double-checking whether these UIs being forms rather than menus would impact whether or not we need a dedicated 'Apply' button. I appreciate the mockup above doesn't include an operator UI. I'll work on that if we decide to refactor in this direction. |
oandregal
force-pushed
the
add/not-in-operator
branch
2 times, most recently
from
a6bd57b to
bb6935b
Compare
I'm no longer waiting for 56514 to be merged first. This one is ready and I welcome reviews. |
|
I know the That being said, there are a few things I'll flag, some of which are more important than others. Consistency
Might feel like a minor point, but the "selected" check marks are inconsistent across menus. The menu that opens from table headers has them on the right, but then on the left when in the Settings submenu. The menu that opens from a filter button however has them consistently on the left, even for the same filter options. Note also that there's a gap on the right of the filter options in the table menu, where icons or other markers might appear. Focus managementNot an issue introduced here, but noting nevertheless. Sometimes, when selecting an item in a table header menu, focus is lost to the winds. This seems to be a latency issue, because I can only reproduce it with fresh filter settings. If I filter or sort by a previously requested value the data is populated immediately and focus remains on the header element. SemanticsDifferent platforms have different considerations when it comes to interdependent menu options, so it's not a huge deal if we use check marks to denote selected items in a mutually exclusive group. 
However, the underlying semantics should be correct, regardless of the visuals. The options in the main filter menu, for example, are (currently) single-select, but have a role of The same is also true of options in the Settings submenu, and (although unrelated to this PR) the sorting options, which have a role of |
General inconsistencies between dropdownmenus are likely caused by the fact that some menus are still using the older version of the component — that should be fixed soon, so probably we shouldn't worry about it in this PR. |
oandregal
force-pushed
the
add/not-in-operator
branch
from
bb6935b to
0318ba3
Compare
|
@andrewhayward I pushed some changes to fix the following things:
|
|
Agree with @ciampo on the dropdown, let's focus on making the new component bullet-proof in separate issues / PRs. @oandregal this is working well. The words are a little tricky, especially as we'll need to account for any/all operators down the road too. Here's a mockup to try, but I'd welcome more feedback: 
Naturally in this PR we should strip that back to: 
I noticed WooCommerce use the term "Conditions" in their product filter widget. I don't love it, but I'm assuming it has stood the test of time for them. I'll finish up by saying these details are easy to tweak after the fact if needed, and don't need to hold up merging this PR imo. |
Thanks. Don't know if this is something to be fixed here or in #56676, but every |
| { filterInView.operator === OPERATOR_IN | ||
| ? __( 'Is' ) | ||
| : __( 'Is not' ) } | ||
| <Icon icon={ chevronRightSmall } />{ ' ' } |
There was a problem hiding this comment.
Should we check for isRTL in all these chevronRight usage?
There was a problem hiding this comment.
Ideally this would be handled at the component level, as in RTL environments (almost) everything should be reversed, with the suffix on the left, facing the other way.
This should just work in the newer menu component.
There was a problem hiding this comment.
I know that we have automatic inversions for CSS but so far, for icons we had to do it manually, so we should be consistent: either have the components handle this everywhere or not, because otherwise it will confuse developers
There was a problem hiding this comment.
Sorry, I wasn't clear – "everything should be reversed" within the context of menu items. The newer menu component doesn't need a chevron icon passed through as a suffix at all; that bit is automatic if the the menu item has a child menu.
Otherwise, yes, RTL icons aren't always just flipped versions of their LTR equivalents, and we should leave that decision to developers.
There was a problem hiding this comment.
The ariakit-based version of the new DropdownMenu component already handles flipping the chevron icon in RTL environments (see storybook example), so I'd say no need to deal with it in this PR
There was a problem hiding this comment.
Mostly unrelated, but as a side-note, it would be great if the icon system was a bit smarter, and could handle bi-directionality implicitly. So rather than developers having to explicitly think about context...
if ( isRTL() ) {
<Icon icon={ icons.formatRtl } ... />} else {
<Icon icon={ icons.formatLtr } ... />}...they could just do something like this...
<Icon icon={ icons.format } ... /> {/* RTL */}
<Icon icon={ icons.format } ... /> {/* LTR */}</brain-dump>
There was a problem hiding this comment.
Would something like this be too rudimentary?
[dir=ltr] {
--icon-direction: 1;
}
[dir=rtl] {
--icon-direction: -1;
}
.icon {
transform: scaleX(var(--icon-direction));
}
There was a problem hiding this comment.
Would something like this be too rudimentary?
Yes, because it doesn't always work.
We could be a bit smarter in the SVG markup though, if we wanted to keep it out of the JS...
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="-2 -2 24 24">
<Path data-dir="ltr" d="..." />
<Path data-dir="rtl" d="..." />
</SVG>[dir=rtl] .icon [data-dir=ltr],
[dir=ltr] .icon [data-dir=trl] {
display: none;
}
As also commented in #56676 (review), the new |
oandregal
force-pushed
the
add/not-in-operator
branch
from
de919f1 to
f4a16d1
Compare
|
It seems that I've addressed all feedback here, so I'm merging this one on Monday if no one else brings up anything. |
This reverts commit 24e5ef3c261c3c10f14735b7fde747b3ee382e87.
oandregal
force-pushed
the
add/not-in-operator
branch
from
f4a16d1 to
c493dbf
Compare
| import { ENUMERATION_TYPE, OPERATOR_IN } from './constants'; | ||
| import { ENUMERATION_TYPE, OPERATOR_IN, OPERATOR_NOT_IN } from './constants'; | ||
|
|
||
| const operatorsFromField = ( field ) => { |
There was a problem hiding this comment.
Wondering if a better name might be sanitizeOperators.
| ) { | ||
| return sprintf( | ||
| /* translators: 1: Filter name. 2: Filter value. e.g.: "Author is Admin". */ | ||
| __( '%1$s is %2$s' ), |
There was a problem hiding this comment.
I wonder how more synthetic languages are handled in this sort of UI (e.g. Notion). I'm not discouraging this bit of string interpolation, just pointing out that we should see how others have done this, so that later we are sure that this UI is localisable.
There was a problem hiding this comment.
At some point, the proposal was Field: value. Though it didn't scale well when you have more operators (in, not in, starts with, etc.).
This is how notion does it (suffers from the same issue):
Gravacao.do.ecra.2023-12-14.as.10.14.24.mov
This is linear (same issue):
Gravacao.do.ecra.2023-12-14.as.10.16.52.mov
The same in other platforms (github, etc.).
I find synthetic languages difficult for this use case because the individual parts (field name, value) are translated separately, and you have no context what they are when translating this string as a whole. Happy to look at examples to improve it.
There was a problem hiding this comment.
This is German:
Gravacao.do.ecra.2023-12-14.as.10.25.31.mov
The gender does affect other parts of the sentence, though the general structure is the same – I'd think what we have here is fine.
There was a problem hiding this comment.
Thanks for having a look! Yeah, I think it's doable, and at worst there might be a handful of languages in which the UI reads slightly awkwardly but remains understandable.
| if ( ! columnOperators || ! Array.isArray( columnOperators ) ) { | ||
| columnOperators = [ OPERATOR_IN, OPERATOR_NOT_IN ]; | ||
| } | ||
| const operators = columnOperators.filter( ( operator ) => | ||
| [ OPERATOR_IN, OPERATOR_NOT_IN ].includes( operator ) | ||
| ); | ||
| if ( operators.length >= 0 ) { |
There was a problem hiding this comment.
Just noticing that this is the same pattern of validation as seen in ./filters.js
Also, is this condition right? It will always evaluate to true, as it's the length of an array.
operators.length >= 0There was a problem hiding this comment.
This is a bug, thanks for catching! Fix at #57048
There was a problem hiding this comment.
On the topic of code reuse: there's an opportunity to reuse some code between the filters.js and the view-table.js. I gave it a try at #56514 though it wasn't merged. I'm happy to pair on this and take a look at how it could be improved.
There was a problem hiding this comment.
Don't worry, I think premature consolidation is a danger. I was just pointing out the pattern for later. :)
There was a problem hiding this comment.
Prompted by this conversation, I've taken a look and consolidated little details so all the filter implementations are mostly lookalike (but still maintaining the little UI/interaction differences for each one) #57059
It's not as ambitious as 56514 was, but I've found some bugs with the column filters by doing this exercise, so it's worth having anyway.
Part of #55083
Related to #55100
What?
Adds support for
NOT INoperator in theAuthorfilter while leavingStatusas it is (only supportsINoperator).Gravacao.do.ecra.2023-11-30.as.17.37.54.mov
Why?
It's part of the interactions we want to achieve.
How?
type: ENUMERATIONgets the filter with both operator (IN, andNOT IN).filterBy.operatorsproperty (see how theAuthorfield uses it to only enable theINoperator).Testing Instructions