DataViews: iterate on list view #56746
Conversation
oandregal
requested review from
youknowriad,
ntsekouras,
jameskoster,
SaxonF and
jorgefilipecosta
oandregal
added
[Type] Experimental
|
@jameskoster @SaxonF I've started looking into updating a bit the list view design. Is there any mockup available? |
| @@ -28,26 +28,17 @@ export const VIEW_LAYOUTS = [ | |||
| label: __( 'Table' ), | |||
| component: ViewTable, | |||
| icon: blockTable, | |||
| supports: { | |||
There was a problem hiding this comment.
This means there is no longer a need to export anything from dataviews (VIEW_LAYOUTS or any specific getViewSupport function).
oandregal
force-pushed
the
update/view-list-preview-prop
branch
from
07c70b7 to
2817be2
Compare
|
Size Change: +681 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
update/view-list-preview-prop
branch
from
fc16708 to
df7a9bf
Compare
|
Flaky tests detected in 4d0011d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7103878089
|
| !! item.featured_media ? ( | ||
| <Media | ||
| className="edit-site-page-pages__featured-image" | ||
| id={ item.featured_media } | ||
| size={ | ||
| currentView.type === 'list' |
There was a problem hiding this comment.
This switches the current code logic but maintains behavior: for the grid view, try finding the largest size. For any other view, do the opposite.
Note that when this code was introduced the current table view was named list. This string was missed when the views were renamed, should have been table now. So this also fixes a bug in trunk.
| @@ -344,8 +344,7 @@ function ViewTable( { | |||
| const columns = useMemo( () => { | |||
| const _columns = fields.map( ( field ) => { | |||
| const { render, getValue, ...column } = field; | |||
| column.cell = ( props ) => | |||
There was a problem hiding this comment.
We can access the view in the fields API directly, no need to pass it to the field through render. Same for view-grid.js. This change is not necessary for this PR to work.
packages/dataviews/src/view-list.js
Outdated
| <div | ||
| role="button" | ||
| tabIndex={ 0 } | ||
| onKeyDown={ onEnter( item ) } | ||
| className={ classNames( | ||
| 'dataviews-list-view__item', | ||
| { | ||
| 'dataviews-list-view__item-selected': | ||
| selection.includes( item.id ), | ||
| } | ||
| ) } | ||
| onClick={ () => onSelectionChange( [ item ] ) } | ||
| > |
There was a problem hiding this comment.
Have you tried/considered using Button here? Generally better to use existing components rather than reimplementing behaviour.
There was a problem hiding this comment.
Yeah. Other than styling, my main concern was that this component is going to have buttons within (this, but also actions).
There was a problem hiding this comment.
Hmm... OK. In which case, we shouldn't be doing this at all! Will need to have a think about how best to handle this scenario.
There was a problem hiding this comment.
Hopefully it's okay to 'visually nest' those buttons, with either absolute positioning, something like the List view implementation, or some other technique?
There was a problem hiding this comment.
Hopefully it's okay to 'visually nest' those buttons, with either absolute positioning, something like the List view implementation, or some other technique?
Yeah, it's a pretty well established paradigm to visually overlay interactive elements. We just can't nest them in the DOM. For example, this card pattern is routinely used on news sites and in other related content...
There are two interactive elements in this card – links for the article title, and the the article category.
However, through some clever use of generated content, and a bit of z-index magic, the entire card becomes clickable, and behaves as if it's all one big link, unless you intentionally hover over the category link, which then becomes the target action.
We don't have to implement it exactly like that, but the general idea is there. One thing to be aware of when doing this is target size – it's bad enough when it's hard to tap on something, but when it's surrounded by a different action context it's even worse.
There was a problem hiding this comment.
Nice, thanks for sharing the details. It'll be useful when we implement this feature.
|
Thanks for working on this, @oandregal. I left a comment about using Outside of that, we should set We should also set |
|
Also looks like #56320 could potentially cause issues here too. |
|
Thanks for taking a look, @andrewhayward !
I've done all of this. Note what I've mentioned about using buttons.
These two are related. The gist of it is that the field that acts as a primary would need to provide some sort of handle, or let the view do that. I could make it work for this page, though I'd rather investigate this separately at make it work for all dataviews. Would that be ok? |
| @@ -48,6 +49,10 @@ const defaultConfigPerViewType = { | |||
| mediaField: 'featured-image', | |||
| primaryField: 'title', | |||
| }, | |||
| [ LAYOUT_LIST ]: { | |||
| primaryField: 'title', | |||
There was a problem hiding this comment.
Maybe the primaryField should be shared by all layouts.. That's a discussion that's not blocking this PR of course..
There was a problem hiding this comment.
Thanks @oandregal! This is definitely a good improvement and I think we can land and iterate.
|
Oh, I think this got merged before all tests were green: the react native test was still running. I set up the "auto-merge" and, upon accepting, the PR got merged directly – without waiting for the test to end. I don't think auto-merge skips the react native tests? I also don't think I've unintentionally clicked the "use admin permissions" to merge directly? I don't see how this happened. 🤔 Anyway, looking at the test failure, it's unrelated to this. I'll keep an eye on Edit: |
@andrewhayward , Is it worth moving this conversation to a new issue to track it separately? And maybe this one too ? |
I'm exploring a potential path for this #56942 |
|
I went through all this PR and updated the follow-up tasks created by Jay to the tracking issue #55083 Please, take a look and share if I've missed anything. |
Part of #55083
Related #56476 (comment)
What?
This PR iterates on the API and design of the list view:
Gravacao.do.ecra.2023-12-05.as.17.53.58.mov
Why?
How?
renderprop of the field API only takes anitemas parameter, no longer takes the view, mimicking howgetValueworks.listview that works with and without preview, so it's not necessary to export whether the view supports previews or not.onSelectionChangeprop toDataViewscomponent that relays the selection to the consumer, to update the preview.Testing Instructions
Follow-ups