DataViews: Add data picker component #70971
Conversation
talldan
requested review from
youknowriad,
ramonjd,
tellthemachines,
andrewserong and
aaronrobertshaw
talldan
added
[Type] Enhancement
|
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. |
|
Size Change: +575 B (+0.03%) Total Size: 1.91 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d94c40b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16616870058
|
|
Nice work @talldan In manual testing selection is working well, and how I'd expect it. Thanks for getting things started. The only question that sprung to mind in relation to the abstraction — bearing in mind I'm still a Dataviews dilettante — what would be an example of its usage in conjunction with Dataviews? Would consumers pick one component or the other or would the I know this was the feedback to create a new component. It just makes me wonder how it all squares with Dataviews' already built-in selection functionality and if we're building "around" an inadequacy. Sorry if I'm way off, just typing things out 😄
My 2c: I think we'd need to first answer whether users expect "Select all" to work across paginations at all. Speaking only for myself, when I see "Select all" on a page, I understand it to mean "select all items currently visible" or, "I know I'm working with what I can see". As far as I can tell this is the standard pattern in apps like Gmail and GitHub 🤷🏻 If the Datapicker allows bulk operations on filtered subsets then that's already a powerful feature, so, unless there's a requirement with sound rationale, I personally don't believe the complexity is worth it. |
+1, also looking at the PR, I do like the idea of an opinionated While testing, I felt like we also need the "cog" icon / sort by drop down that DataViews uses, too? Another challenge with having separate views, is that I imagine if this component is used for something like a post parent picker or a modal for selecting page links, etc, that we'd also really want a Table view for browsing and selecting while viewing more of the fields. So if we have (semi) duplicate Table and Grid views, will that be a challenge for maintenance / ensuring things are always compatible?
Yes, personally I'd expect "Select all" to only refer to the items currently in view, and not span across pagination. |
Rather than having the separate
The way it works right now you can still reuse parts of dataviews - the search, pagination are shared, it's the view and the bulk selection parts that are different. I think this is the part where I don't feel the abstraction is quite right in the PR because of the way
Yep, I think we will have duplicates, but then internally/semantically I expect they will be quite different. Also I think there's the potential for the presentation to be very different. We can also try extracting some aspects of the implementation to be usable by both. For example the grid layout might be something we consider extracting so the grids always have the same sizing/responsiveness. |
There was a problem hiding this comment.
I'm very conflicted about this PR 😅
I love how it simplifies keyboard interaction with the component, vs the DataViews component which is simply painful to navigate.
DataPicker is in a weird place because it's both kind of its own DataView and also kind of just a big complex DataForm. It might help to think about (maybe even try out) some different use-cases for it in order to gauge what its best shape would be. Assuming potential uses like media, posts, templates...? What is it going to help us do? Where will it be really necessary to use this component as opposed to a simple (or multi-) select?
I also wonder if its layouts couldn't be a lot more basic than the DataViews ones given the items in this component are much less interactive. Do we really need anything more than image/preview and title? We might not even need different markup for different layouts; in fact, I'm not sure a listbox should be a table at all. Maybe the only thing we need is to switch between list and grid layouts by changing the display on the parent and on the child items 😺
| > | ||
| <div | ||
| className="dataviews-wrapper" | ||
| ref={ useMergeRefs( [ containerRef, resizeObserverRef ] ) } |
There was a problem hiding this comment.
I just merged #70493 which removes resizeObserverRef from this wrapper and passes it into DataViews context instead. In any case, you'll only need it here if you decide to enable the previewSizePicker in this layout.
Also, containerRef is only used in the table layout to allow horizontal scroll on long tables, so you shouldn't need it here either.
I think if the strategy is to duplicate the parts of DataViews and their layouts that the picker needs, it might be best to strip this down to just the bare essentials for now and then add more features as needed, because otherwise it will be too easy for this component to fall out of sync as DataViews changes.
There was a problem hiding this comment.
I've rebased to bring in those changes. I might wait for more feedback before settling on an approach, but I can either strip it back or refactor the grid into something shareable across the two layouts.
| @@ -0,0 +1,105 @@ | |||
| .dataviews-picker-grid { | |||
There was a problem hiding this comment.
Could we not reuse the dataviews-view-grid class in the component and avoid adding these styles? Again, this is already out of sync with trunk due to my earlier merge 😳
If we need to add extra styles to the picker, I think it's fine to give it an extra class name for whatever styles are exclusive to it alongside dataviews-view-grid.
There was a problem hiding this comment.
Possibly, though I also get told not to share classnames across components. 😄
Another option is for the Grid to be extracted into a reusable component.
There was a problem hiding this comment.
I will never understand the need to create a whole JS component just to share CSS between components, when CSS itself has a perfectly good system for that, but I suppose that ship has sailed for this repo 😞
One use case in the back of my mind is to be able to support browsing from and choosing a page parent for sites with a very large number of pages (e.g. enterprise or large knowledge bases). There's an issue in #69608 that the current approach maxes out at 100 pages, so for larger sites I could imagine the DataPicker in a modal being really handy for being able to browse, search and sort before selecting a parent (particularly since it supports pagination). |
|
Choosing media or patterns is another use case in WordPress. We have this modal at the moment for patterns, which doesn't have any kind of searching or pagination, and there might be quite a lot of patterns, especially if we support user created patterns in this view in the future: |
talldan
force-pushed
the
add/data-picker-component
branch
from
792116f to
b4e0504
Compare
talldan
force-pushed
the
add/data-picker-component
branch
from
b4e0504 to
9992c14
Compare
| import type { View } from '../../../types'; | ||
|
|
||
| export const DEFAULT_VIEW: View = { | ||
| type: 'picker-grid', |
There was a problem hiding this comment.
Why is this not just "grid"?
| paginationInfo={ paginationInfo } | ||
| getItemId={ ( item ) => item.id.toString() } | ||
| defaultLayouts={ {} } | ||
| onFinish={ ( ids ) => { |
There was a problem hiding this comment.
onSubmitSelection or something like that?
| startPosition: number; | ||
| }; | ||
|
|
||
| export default function DataPickerGridLayout< Item >( { |
There was a problem hiding this comment.
Does this need to be different that "grid"? I'm trying to understand what are the main differences between the two components, what should be shared or not...?
There was a problem hiding this comment.
The markup, semantics and keyboard navigation are quite different. Also different props.
The parts that are most similar are some of the styles, like the grid columns and responsiveness.
Possibly we could try using the same view, but consider selection a different mode of the view that when active adds the different roles, keyboard nav and so on. It'd need to do quite a lot, like remove all the tabbable elements within items. 🤔
There was a problem hiding this comment.
Possibly we could try using the same view, but consider selection a different mode of the view that when active adds the different roles, keyboard nav and so on. It'd need to do quite a lot, like remove all the tabbable elements within items. 🤔
Yes, this is what I kind of was thinking about, maybe a "layout" can decide whether it can be used in "DataViews", "DataPicker" or both.
Now it doesn't mean that it can't be two separate components, but I feel like the entry point should at least be the same no?
Now, I do think this PR is a bit "fundamental" in the sense that introduces a new component / new behavior entirely for DataViews package. I would love as much feedback as possible as I do have uncertainties myself. cc @WordPress/gutenberg-core @mtias @aduth
There was a problem hiding this comment.
If we were to reuse layouts, why not just use DataViews with a "picker" mode instead of creating a dedicated component? Most (maybe all?) of the custom behaviour we need to introduce here concerns the layout structure, semantics and interaction, as well as the structure and interaction of the items, which is also defined inside the layouts.
For the sake of not adding more complexity to the current view layouts, I like the idea of adding a new "picker" layout - or several, but I'm not convinced picker needs more than one layout component. Certainly its HTML shouldn't change because it needs the listbox semantics, so if we want to customise the display I think it would be better to do it purely with CSS variations.
|
Hey, I'm trying to understand what's this for and why DataViews can't be used. Would this be a good breakdown of what we need in a picker, or did I miss something?
I'd like to offer an alternative direction that leverages the existing DataViews component instead of creating a new one.
Keyboard navigation: both improvements are useful to DataViews grid independently of what we do with pickers, so it'd be good to have them anyway.
Using the existing DataViews, this is where we stand visually (in red what we need to resolve): 
|
|
@oandregal some previous discussions here #70722 (comment) |
|
@youknowriad I saw that but didn't see a breakdown of the specifics. Are we settled on a separate component, or is it open to discussion? Based on the breakdown above and unless I've missed some other task, I don't think it's too hard to make DataViews work in that use case. |
|
I don't think we're totally settled but I'm personally almost there :P The two arguments for me are:
I also know @ellatrix tried to use DataViews list view for a "picker" once (Site picker in calypso I think) and she reverted because it just didn't fit. Curious if you remember the specifics. |
What?
Related - #70722
Adds a
DataPickercomponent to the DataViews package that can be used for picker/selection flows.Not finalized, but I'm looking for feedback before consolidating further on the approach.
On the surface this looks like the 'grid' layout for DataViews with a few differences:
listbox, the list only has one tab stop with left/right arrow keys used for navigation, and space/enter used for selection.How?
DataPickercomponent that internally shares some details of DataViews (views, context, search, pagination), but has fewer props and a more specific purpose. It has a similar set of props to DataViews, but also supports:multiple- is single or multiple selection supportedlabel- the label for thelistbox(required since the list of items is focusable)onFinish- a callback triggered when the user has made a selection.DataPickerGridlayout/view for the DataPicker. In the future we can implement different kinds of selection/picker layouts.Challenges
Todo
Testing Instructions
npm run storybook:devDataPickerstory (once the storybook has built and opens in your browser)Screenshots or screencast
(when clicking finish there's an alert that shows, but it appeared outside the video crop zone 😄 )
Kapture.2025-07-30.at.15.44.34.mp4