DataViews: allow users to add filters dynamically #55992
Conversation
oandregal
requested review from
youknowriad,
ntsekouras,
jameskoster,
SaxonF and
jorgefilipecosta
oandregal
added
[Type] Feature
|
Size Change: -663 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
@jameskoster @SaxonF I wanted to clarify some details about the API and interactions/UI, so I went ahead and implemented this – even if |
|
Thanks for the ping, and the hard work on this so far. In terms of design, I don't think it should be framed as show/hide, instead the general heuristic should be; if the filter is active then it is visible. Otherwise it would be possible to hide active filters which could lead to a fairly confusing UX. So with that said the behavior is pretty close, but the View menu doesn't seem like the right place for this affordance. I still believe the ideal state is a "+ Add filter" button which includes the options, and even allows you to configure the filter right there via flyout: 
Once added, the filter can be removed via the associated dropdown (or by clicking the global "Reset filters" button): 
Do you think that's feasible, or do we need to come up with something for the short-term? If so, perhaps something like this could work: 
In this scenario removing a filter would also reset it.
The original idea was that some filters are so common that they should be included by default. But I feel that we've pivoted a bit on that by including default Views instead, such as "Draft" and "Trash" for pages. So for now we can probably defer adding any special privileges for certain filters, as it may not be needed. I hope that answers all the questions! |
It does, it's very helpful. Thank you, @jameskoster! I'll update the PR to incorporate that feedback tomorrow/Monday. |
oandregal
force-pushed
the
update/filters-show-and-hide
branch
from
65da3bf
to
c5f597b
Compare
oandregal
changed the title
|
@jameskoster I've implemented the suggested design: Gravacao.do.ecra.2023-11-10.as.11.10.56.movThe one thing I see missing is the ability to remove a single filter from their own component. I'd like to do this in a follow-up PR: it requires updating the current filter components to dropdowns, which also enables us to implement multi-select, etc. |
|
I'd expect for filters to show when they have a value. An example is going to |
|
|
||
| export default function AddFilter( { filters, onChangeView } ) { | ||
| return ( | ||
| <BaseControl> |
There was a problem hiding this comment.
Why do we need this BaseControl wrapper?
There was a problem hiding this comment.
This is how it looks like without it (alignment issues):
Happy to solve it any other way.
There was a problem hiding this comment.
BaseControl is mean to add a label and a help to controls (inputs, selects...), if it's the spacing that is the issue here, we should probably just use a HStack with align center or something for the row or something like that.
There was a problem hiding this comment.
I've already tried setting the alignment in the existing HStack component, though it didn't work.
There was a problem hiding this comment.
Note the alignment of the components in the top-level is an issue for the "view actions" component as well (pre-existing to this PR):
There was a problem hiding this comment.
There was a problem hiding this comment.
Found a way to reset the BaseControl margin via component prop at 353ca40 I'll mark this as fixed.
You mean selecting one of the default views in the sidebar? That's a good one, I'll fix on Monday. |
oandregal
force-pushed
the
update/filters-show-and-hide
branch
from
1afeb01
to
b959096
Compare
|
@ntsekouras sidebars behaviour is fixed now. |
| @@ -47,7 +47,6 @@ Example: | |||
| { field: 'author', operator: 'in', value: 2 }, | |||
| { field: 'status', operator: 'in', value: 'publish,draft' } | |||
| ], | |||
| visibleFilters: [ 'author', 'status' ], | |||
There was a problem hiding this comment.
We no longer need this, the visible filters are derived from the filters state.
|
Flaky tests detected in bbda883b3c0b9d7d12150ba85ba0c0ff6e6e94a7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6862700891
|
|
@jameskoster @ntsekouras is there any blocker to merge this PR? I've got some follow-ups ready that would benefit from having this in |
| label={ __( 'Add filter' ) } | ||
| trigger={ | ||
| <Button | ||
| aria-disabled={ filters.length === view.filters?.length } |
There was a problem hiding this comment.
Should this be __experimentalIsFocusable disabled like suggested by @ciampo
There was a problem hiding this comment.
I always thought this behavior should be default in buttons, any reason to require __experimentalIsFocusable @ciampo ?
There was a problem hiding this comment.
Unfortunately I don't have much context around it, since these changes were discussed before I started working on @wordpress/components. Best I could find is this comment — it looks like we initially wanted to make the prop true by default, but we were concerned about potential unintended consequences.
There was a problem hiding this comment.
Good call and I was on that discussion as well. I guess now, I may be more on the side of let's try and remove that prop and make it default and see how it goes.
|
Ok, this is the latest: the reset button behaves as it was, this PR only adds the new "+ Add Filter" button. The video demonstrates a bit of keyboard navigation and filtering through different means (sidebar, columns, top-level components): Gravacao.do.ecra.2023-11-15.as.11.16.51.mov |
|
A couple of thoughts on the general UX... The + Add filter menu has a disabled Reset button when no filters are active. Do we really need this, isn't it reasonable to only render this button when filters are active? Similarly when a filter has been added it still appears in the + Add filter menu, disabled. This convention adds a lot of noise making it less clear which filters you can add to the existing set. Since the button label is "Add filter", isn't it fair to only display filters the user can add and hide active ones? When adding a filter like Author or Status, the "All" value isn't super-helpful. Would it be possible to hide that option? One tiny detail; let's use the |
I've just updated this. Note that for focus control, the "Add Filter" button is not hidden but disabled (when no more filters are left to add).
I'll remove the filter from the list. Edit: done at e9c62fb
I'll remove as well. Done at fb601d2
I wonder if you have tried with the latest PR? This is how it looks now: 
|
|
@jameskoster I think all your feedback has been addressed :) Gravacao.do.ecra.2023-11-15.as.11.43.23.mov |
Ah, I misunderstood this. It's now fixed: 
|
I'll look at that separately. |
Using Looks like we need to check the focus ring styling on
Seems like the CSS is intentionally overriding the custom focus ring to remove it. Ideally it would be the same, but at the very least we should be indicating focus somehow.
|
One thing to be aware of when removing buttons is focus management. If a user presses the "reset" button, and there are no active filters remaining (it's unclear if it resets the filters to their "saved" state, or removes them entirely), the button will be removed. This then means that the focus, which was on the button, is lost, which can be confusing. If we are going to go ahead with this, we'll need to make sure focus is at least moved somewhere appropriate – possibly the "add filter" menu button. |
This has been changed so the "ResetFilter" button is independent of the "AddFilter" component and always displayed. This is the latest: Gravacao.do.ecra.2023-11-15.as.11.43.23.mov |
For reference, I filed #56149 to handle this. |
Bugfix at #56151 |
oandregal
added
[Type] Experimental
Part of #55083
Related #55100
What?
Allow user to add filters dynamically.
Gravacao.do.ecra.2023-11-15.as.11.43.23.mov
Testing Instructions