DataViews: add filters to table columns #55508
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
requested review from
ntsekouras,
mcsf,
youknowriad,
jorgefilipecosta and
ellatrix
oandregal
added
[Type] Feature
oandregal
commented
Oct 20, 2023
| @@ -49,7 +52,8 @@ const sortingItemsInfo = { | |||
| desc: { icon: arrowDown, label: __( 'Sort descending' ) }, | |||
| }; | |||
| const sortIcons = { asc: chevronUp, desc: chevronDown }; | |||
| function HeaderMenu( { dataView, header } ) { | |||
| // TODO: why doesn't Header work with view/onChangeView? | |||
| function HeaderMenu( { dataView, header, view, onChangeView } ) { | |||
There was a problem hiding this comment.
I've noticed the HeaderMenu doesn't take the fields, view, onChangeView as properties, but uses tanstack instead. Can we implement this component's functionality by only using our own abstractions?
Having to use two things for the same operations is confusing. For the demo, I chose to just pass view and onChangeView, but we need to decide how this works.
There was a problem hiding this comment.
Set to use the existing dataView, header. If this needs change, it'd be done in a different PR.
oandregal
commented
Oct 20, 2023
|
Size Change: +463 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
oandregal
commented
Oct 20, 2023
|
Flaky tests detected in 5bb04fe. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6590856011
|
|
@SaxonF @jameskoster What do you think we should do with filters that are not present at the top-level? This is the use case: let's say a certain filter is not visible in the top level, but the column filter is. In this PR, you can see this at work by using the column filter "Author is not". If the user filters by "Author is not" which affordance should we use to communicate the filter status to the user? I was thinking we could add a token-like element for this case, like in this earlier exploration. It sounds it could work as it communicates the dataset is filtered, and the user can remove it from the top-level: 
What would be your thoughts? Note that this is also a question that we'd have to answer when the top-level filters can be hidden by the users. This is how to replicate in that scenario:
|
|
Imo filters should always be visible if active, and hidden if inactive (except for primary filters). We should also use the same UI, not a mix of dropdowns / chips. The I put a prototype together that (hopefully) captures what we want to accomplish in this PR: filters.mp4 |
|
@oandregal Unsure if this is something to handle here or separately, but worth noting that some filters could have many terms, so we'll need scrollable lists, searching, and potentially loading states: tags.mp4 |
|
ok, I can limit this PR to just one filter per field and make it just about introducing column filters. @jameskoster For the pages page, there are opportunities to have many filters per field: for example, Author ("in" and "not in") and Date ("before" and "after"). It'd be beneficial to have something for any of those soon to stress-test the filters API and make sure we're not missing anything. |
26 tasks
Absolutely. The design of the individual "Enumeration" filter component is something that needs a lot of work, and can be done in parallel. |
|
Yes date is a particularly complex example. You might want to view pages published in a specific year, month, week, day, or time. Then you might want to view pages published before and/or after selected date(s) as you mentioned. I'll work on some designs for that next. Fortunately Author and Status are a bit simpler so hopefully we can prioritise those. |
oandregal
force-pushed
the
add/dataviews-filters-in-columns
branch
from
4a95044
to
5bb04fe
Compare
|
Interaction-wise, this has been limited to one filter of type Gravacao.do.ecra.2023-10-20.as.19.27.07.mov |
|
Nice! If possible as a part of this PR, it would be good to:
|
I'd like to address those as follow-ups. They can be worked on parallel and be picked up by anyone. UI is going to need a lot of focused efforts, and I'd rather have the basic interactions working first. |
oandregal
force-pushed
the
add/dataviews-filters-in-columns
branch
2 times, most recently
from
f5b40b2
to
f458649
Compare
oandregal
force-pushed
the
add/dataviews-filters-in-columns
branch
from
f20cf5f
to
8f812dd
Compare
|
Rebased to solve some issues. Would welcome a review @youknowriad @jorgefilipecosta @ntsekouras :) |
youknowriad
reviewed
Nov 6, 2023
| @@ -63,6 +67,29 @@ function HeaderMenu( { dataView, header } ) { | |||
| return text; | |||
There was a problem hiding this comment.
Just a small non blocking remark: This component HeaderMenu is built as a tan stack component (gets dataView and header) and renders a dropdown. I wonder if it could be similar to built it as a DataView component instead. (get the view and field as props).
There was a problem hiding this comment.
I had the same thought #55508 (comment)
youknowriad
reviewed
Nov 6, 2023
That reminds me – we're missing a global reset button for filters. 
Appreciate it's one to handle separately but wanted to leave a note anyway. |
Nice one, fixed at a9c651b |
oandregal
commented
Nov 6, 2023
| @@ -427,12 +427,13 @@ function ViewList( { | |||
| } ); | |||
| }, | |||
| onGlobalFilterChange: ( value ) => { | |||
| onChangeView( { ...view, search: value, page: 0 } ); | |||
| onChangeView( { ...view, search: value, page: 1 } ); | |||
There was a problem hiding this comment.
Apparently, this value is not being used – or is ignored once the own Search component updates it to 1 here. I thought I'd change it anyway, though this needs a separate further investigation.
Would you think we could use the existing #55100 for tracking filter-related work? I can help to maintaining it up to date as we go, if you want. |
youknowriad
approved these changes
Nov 6, 2023
|
I've updated the existing filter issue to track ongoing work #55100 Hopefully, each task comment is self-explanatory (which ones need design, which need component work, etc.). |
oandregal
changed the title
oandregal
added
[Type] Experimental
oandregal
changed the title
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
Follow-up to #55270
What?
This PR adds support for column filters:
Gravacao.do.ecra.2023-11-02.as.18.16.03.mov
Testing Instructions
listtype view, interact with the columns that have filters.