DataViews: Redesign of filters #58569
Conversation
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
mcsf,
oandregal,
jameskoster,
SaxonF,
ciampo and
jasmussen
|
Size Change: +17 kB (+1%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Nice start, thanks for working on this 👏 I haven't looked deeply into the changes yet, but I've got one question: is this to be released in 6.5? If yes, that's great – let's ship this one fast so we can iterate. If, on the contrary, it's deemed not ready, I'd ask to do the changes in a way that they can co-exist with the existing implementation: we don't want to shoot ourselves in the foot and make it harder than necessary to address bug/modifications to the existing implementation if that's still shipping in 6.5. |
We have one week left to get this in. So I'm hoping yes, but we'd need reviews and designs and a11y feedback. |
|
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 Core SVNCore Committers: Use this line as a base for the props when committing in SVN: GitHub Merge commitsIf 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. |
| > | ||
| <div className="dataviews-search-widget-filter-combobox__input-wrapper"> | ||
| <Ariakit.Combobox | ||
| autoSelect |
There was a problem hiding this comment.
I tried to pass SearchControl to render prop here, but there is an issue because Ariakit expects always an event at the onChange callback(destructures from event.target), so we can't use in combination with setValueOnChange prop set to false.
For this reason I copied some html structure and styles from SearchControl, but also placed the icon at the left side.
There was a problem hiding this comment.
Maybe there's a way for us to hack around that — ie. something like this?
const HackedSearchControl = ( { onChange, ...props } ) => {
const [ inputRef, setInputRef ] = useState( null );
useEffect( () => {
inputRef?.addEventListener( 'change', onChange );
return () => {
inputRef?.removeEventListener( 'change', onChange );
};
}, [ inputRef, onChange ] );
return (
<SearchControl { ...props } ref={ ( node ) => setInputRef( node ) } />
);
};
// ... later
<Ariakit.Combobox
autoSelect
placeholder={ __( 'Search' ) }
className="dataviews-search-widget-filter-combobox__input"
render={ <HackedSearchControl /> }
/>
Haven't tried it on my machine, though — but it could be an idea
packages/dataviews/src/filters.js
Outdated
| f.operator | ||
| ) | ||
| ( f ) => f.field === field.id | ||
| // && |
There was a problem hiding this comment.
@oandregal can you help me understand the commented check? Is it needed?
There was a problem hiding this comment.
It checks the operator in use by the view is one of the available/registered operators. It essentially prevents filters with no operators (or with unknown operators) from being displayed. Operators are a required field in the view.filters, this check enforces that requirement.
Imagine the following situation:
- We store the filter in the view without operator, assuming the default in the UI is
IS. - For whatever reason, the UI changes the default to
IS NOT. At this point, all the filters that come from the view without operator would have their results inverted (custom views from database, default views provided by WordPress, etc.).
There was a problem hiding this comment.
In summary, we still need it :)
There was a problem hiding this comment.
This is worth adding as a comment, as it's not immediately clear how it is important.
|
This is exciting to see :) A couple of UX notes:
Some of the visuals will need tweaking, but I can probably handle most of that. In the filter chips/buttons would it be possible to wrap the selected values in an element so that we're able to visually differentiate the filter from the value? E.g.:
|
|
This is one of those situations where moving focus is legitimate and useful, putting the user in a place where they can complete the intended action. moving.focus.when.adding.filter.movSomething like this would work...Note: This patch breaks focus on escape for some reason! diff --git a/packages/dataviews/src/filter-summary.js b/packages/dataviews/src/filter-summary.js
index b6857b60e2..b699447f0c 100644
--- a/packages/dataviews/src/filter-summary.js
+++ b/packages/dataviews/src/filter-summary.js
@@ -9,6 +9,7 @@ import {
FlexItem,
SelectControl,
} from '@wordpress/components';
+import { useRef } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
/**
@@ -135,13 +136,16 @@ function ResetFilter( { filter, view, onChangeView, filters } ) {
}
export default function FilterSummary( props ) {
- const { filter, view } = props;
+ const { filter, view, openOnMount = true } = props;
+ const defaultOpenRef = useRef( openOnMount );
const filterInView = view.filters.find( ( f ) => f.field === filter.field );
const activeElement = filter.elements.find(
( element ) => element.value === filterInView?.value
);
return (
<Dropdown
+ defaultOpen={ defaultOpenRef.current }
+ focusOnMount={ false }
contentClassName="dataviews-filter-summary__popover"
popoverProps={ { placement: 'bottom-start', role: 'dialog' } }
renderToggle={ ( { onToggle } ) => (
diff --git a/packages/dataviews/src/filters.js b/packages/dataviews/src/filters.js
index 2bb6ce7a6c..c12c81e45a 100644
--- a/packages/dataviews/src/filters.js
+++ b/packages/dataviews/src/filters.js
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
-import { memo } from '@wordpress/element';
+import { memo, useState } from '@wordpress/element';
/**
* Internal dependencies
@@ -49,6 +49,8 @@ const Filters = memo( function Filters( { fields, view, onChangeView } ) {
} );
}
} );
+ const visibleFilters = filters.filter( ( filter ) => filter.isVisible );
+ const [ initialFilters ] = useState( visibleFilters );
const addFilter = (
<AddFilter
@@ -60,21 +62,16 @@ const Filters = memo( function Filters( { fields, view, onChangeView } ) {
);
const filterComponents = [
addFilter,
- ...filters.map( ( filter ) => {
- if ( ! filter.isVisible ) {
- return null;
- }
-
- return (
- <FilterSummary
- key={ filter.field }
- filter={ filter }
- view={ view }
- onChangeView={ onChangeView }
- filters={ filters }
- />
- );
- } ),
+ ...visibleFilters.map( ( filter ) => (
+ <FilterSummary
+ openOnMount={ ! initialFilters.includes( filter ) }
+ key={ filter.field }
+ filter={ filter }
+ view={ view }
+ onChangeView={ onChangeView }
+ filters={ filters }
+ />
+ ) ),
];
if ( filterComponents.length > 1 && view.type !== LAYOUT_LIST ) {
diff --git a/packages/dataviews/src/search-widget.js b/packages/dataviews/src/search-widget.js
index fba875cdc3..14b524caef 100644
--- a/packages/dataviews/src/search-widget.js
+++ b/packages/dataviews/src/search-widget.js
@@ -73,6 +73,8 @@ export default function SearchWidget( { filter, view, onChangeView } ) {
>
<div className="dataviews-search-widget-filter-combobox__input-wrapper">
<Ariakit.Combobox
+ // eslint-disable-next-line jsx-a11y/no-autofocus
+ autoFocus
autoSelect
placeholder={ __( 'Search' ) }
className="dataviews-search-widget-filter-combobox__input" |
There was a problem hiding this comment.
Thank you for working on this, Nik!
It uses some lower level components from
@ariakit, because eventually the goal is to port it under the@wordpress/componentspackage.
I don't think that the whole "popover with filter controls" component has a place in @wordpress/components, given how high-level and specific it is to the Data Views project.
What we should iterate, and then extract if possible, are the individual components — like, potentially, an update to ComboboxControl which is currently not flexible enough to accommodate Data View needs.
|
Flaky tests detected in e551120. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7816992036
|
ntsekouras
force-pushed
the
redesign/dataviews-filters
branch
from
d1713d3
to
723f300
Compare
|
Regarding the focus on the new filter: (@andrewhayward )
Unfortunately this wouldn't work, because we also have custom views, which could have more filters saved. What this means is that initially we show the filters based on whether they are I explored keeping the info for opening on mount in store, with dispatching actions on the menu items that add a filter. |
|
I pushed a couple of tweaks, mostly cosmetic to get the filter config appearance closer to the new dropdown design. @ntsekouras A couple other detail to address;
focus.mp4 |
|
I tested a few flows via the keyboard (including filtering from columns, primary/secondary, etc.) and this works nicely. This is one thing I ran into: the first time you add a filter, there is a focus loss after you close the search widget (either via ESC or by clicking remove): Gravacao.do.ecra.2024-02-05.as.18.07.09.movGravacao.do.ecra.2024-02-05.as.18.06.18.mov |
|
Some general feedback about the interactions (not blocking, can be follow-ups if we want to do them). Have we explored adding the dynamic filters after the "Add Filter" & "Reset Filter" buttons? It could be: "+Add filter" > "Reset filters" > "Primary filters" > "Secondary filters". This provides a stable and short length for common operations: reset & add. In situations with more filters (say, 3 or 5), users would have to tab them all before they can reset them or add a new one. One thing I miss from the existing implementation is how quickly selecting an option was. This requires a few more jumps: click to add filter, tabs to go to search, arrow down to select item. I'd love if we could make it more streamlined for keyboard. Some suggestions:
|
|
Also, in testing this PR, I've noticed that the primary filter PR was merged leaving "status" as primary for pages. #58682 removes it. |
|
I've also noticed that there is no "Reset button" for the list layout. (enable the experiment, go to Pages > Manage all pages and switch to list layout). |
ntsekouras
force-pushed
the
redesign/dataviews-filters
branch
from
fcde2aa
to
6dc16b4
Compare
|
I believe this is something that could land in order to be included in 6.5 and iterate small issues/fixes if we need to.
@jameskoster I think this is the remaining request of yours, that I'd like to do in a follow up and explore the best way to do it, as it's not as simple as hiding the combobox. We would need a different a11y pattern for that(probably lisbox) and might need some discussion/exploration first. What do you think? |
| } | ||
| } | ||
|
|
||
| .dataviews-search-widget-filter-combobox-list { |
There was a problem hiding this comment.
I'd like us to have a hierarchical/structured naming schema for dataviews. Something like .dataviews-component-subcomponent that resulted in an obvious name for this like .dataviews-filters-search-*, etc. At the moment, it's very ad-hoc.
It'd be great if at least the names introduced in this PR followed the same pattern. If it's too much to ask, we should address afterwards – it's not something we cannot change.
There was a problem hiding this comment.
That sounds great to do. @oandregal is this something you can do in a follow up? Alternatively I will do in the next days for 6.5
| { canAddFilter && ( | ||
| <DropdownMenuGroup> | ||
| <DropdownMenuItem | ||
| prefix={ <Icon icon={ funnel } /> } |
There was a problem hiding this comment.
Shouldn't we be consistent and use the + icon here as well? Funnel made sense when it was everywhere the same.
There was a problem hiding this comment.
Nice work here!
It's a nice refactor for the filter UI 👏 There are some flows/interactions that could be streamlined to improve the add/reset filter user actions, but this provides a good base to work from: #58569 (comment)
I haven't found any blockers, though given the size of the PR I've probably missed little opportunities to improve. Let's get this in 6.5 and iterate.
|
@ntsekouras that seems okay to explore separately. The other request was this one:
Also fine to handle separately, but I wanted to make sure it was noted. Most importantly; if we merge this for 6.5 we'll need to update the designs for the filter chips. I shared a concept for that here yesterday. Do you think that would be feasible? Edit: I'm not saying we need to do that in this PR, but it would be a shame for the new design to land in 6.5 without it. |
|
Thanks for all the help here! I landed it and let's follow up quickly with the refinements. |
What?
Part of: #55100
This PR redesigns the DataViews filters based on: #55100 (comment).
Based on the designs we need a new widget, as we don't have something similar in the
@wordpress/componentspackage. In order to give us time to explore a good solution that can satisfy our use cases, the new component is added directly in the DataViews package. It uses some lower level components from@ariakit.This PR needs polishing(mostly design) and feedback.
Tasks
Screenshots
Screen.Recording.2024-02-05.at.9.40.09.AM.mov
Testing instructions