DataViews: Support hiding the title in Grid layout, with actions available on hover #71369
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +360 B (+0.02%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bd4b9f4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17389717828
|
andrewserong
force-pushed
the
try/titleless-dataviews-grid-with-hover-actions
branch
from
94db3db to
0eca750
Compare
andrewserong
requested review from
jameskoster,
youknowriad,
talldan,
ramonjd and
tellthemachines
|
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. |
andrewserong
changed the title
andrewserong
changed the title
There was a problem hiding this comment.
Works mostly well for me in testing. I noted that when using voiceover, the checkbox tells you the title of the item, so the title isn't completely lost, which is good.
When using the story I note that it's now possible to hide absolutely everything and items become completely invisible. I know this was discussed already, but I feel like that's not a great situation, I don't know if there was a conclusion to the discussion:
It'd be good to add some tests. In particular a test that checks the checkboxes and actions dropdowns are accessible through the tab key would be great, as it's the kind of thing that ends up as a bug as people tinker with the styles.
packages/dataviews/src/components/dataviews/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
| .dataviews-view-grid__card:hover .dataviews-view-grid__hover-actions, | ||
| .dataviews-view-grid__card:focus-within .dataviews-view-grid__hover-actions { | ||
| opacity: 1; | ||
| } | ||
|
|
||
| // Include an explicit .is-hovered class to preserve hovered state when popovers are active. | ||
| .dataviews-view-grid__card .dataviews-view-grid__hover-actions.is-hovered { | ||
| opacity: 1; | ||
| } |
There was a problem hiding this comment.
May also want to consider ensuring opacity: 1 is used when aria-expanded is true on the opener button. At the moment the button will hide itself. focus-within doesn't work consistently because the dropdown menu content isn't rendered in the same part of the DOM.
I also wonder if the selection checkbox should have matching visibility logic, so all the interactive elements for an item are always visible at the same time - e.g. whenever the dropdown is open the checkbox is also visible. Though the table layout's UI doesn't work like this so maybe not. It's a very minor thing so maybe not worth sweating about!
| @@ -18,7 +18,7 @@ const titleField: Field< CommonPost > = { | |||
| placeholder: __( 'No title' ), | |||
| getValue: ( { item } ) => getItemTitle( item ), | |||
| render: TitleView, | |||
| enableHiding: false, | |||
| enableHiding: true, | |||
There was a problem hiding this comment.
I understand we want to display actions in the media field when title is not present. But, do we also want the title field to be hidable in the existing views (site editor's pages)? Not anything against it, just double-checking.
There was a problem hiding this comment.
But, do we also want the title field to be hidable in the existing views
Yes, it'll be a key part of building out a new media library that's powered by data views, as we'll need to be able to hide the title for attachment post types in a grid view.
There was a problem hiding this comment.
Yeah, that sounds a good experience for the media library, but can this be addressed differently without affecting existing views? For example, with this view config for the media library:
const view = {
type: 'grid',
// titleField not provided
mediaField: '...',
fields: [ /**/ ]
};That way, we wouldn't enable hiding the title in the site editor's page.
There was a problem hiding this comment.
but can this be addressed differently without affecting existing views?
No, I don't think so. For the media use case, we still want the title field to exist, as it's needed for accessibility (i.e. the label provided to the selection checkbox). So it's important that we can hide the title field, without excluding it altogether from the view.
Also, I think it should still be possible for folks to show the title field if they want to, with it defaulted to hidden.
To mitigate the issue for existing views, that's one of the reasons I was proposing making it so that if any locked fields are present, we can't hide the last one that's visible. So we give users more flexibility, but add a guardrail so that they can't get to an unusable state where no fields at all are visible.
There was a problem hiding this comment.
That way, we wouldn't enable hiding the title in the site editor's page.
Oh, one detail to mention here is that this PR doesn't affect the pages list in the site editor as that uses a different field (pageTitleField), rather than the generic titleField which is used as a fallback here:
There was a problem hiding this comment.
I think it makes sense for the base title to default to enable hiding. If fields can usually be hidden, disabling hiding is a decision which should probably be made at the consumer level. Both Pages and Templates have their own title fields which disable hiding, so nothing will change in the actual site editor.
There was a problem hiding this comment.
Oh, one detail to mention here is that this PR doesn't affect the pages list in the site editor as that uses a different field
Oh, nice. My worry was about affecting existing screens 👍
Little thing (default value is true):
| enableHiding: true, |
disabling hiding is a decision which should probably be made at the consumer level
I think of wordpress/fields as consumer-level code! It's just a different package, but I see it as an opinionated fields bundle for WordPress entities that we expect to use in our screens. While any screen can modify the field, in this bundle, it's best to have good defaults.
Thanks to you both for the context. I now understand this change better.
There was a problem hiding this comment.
Actually, this field is used in the post list experiment ("Gutenberg > Experiments > enable for posts", then visit "Gutenberg > Posts"). Now that I've looked more into how fields are organized, would this work for you instead?
- Leave the existing
titlefield as it is. - When it comes to use the title field in the media library, revisit this. We could/may need to add a
mediaTitleFieldthat has specific field configs. I understand we don't need that now, but this change was more of a future thing, right?
This would prevent making the field hidable for any CPT entity, for example.
In any case, if you still think it's best to carry on with this change, I'd suggest at least adding a changelog entry in the fields package so consumers are aware of this.
There was a problem hiding this comment.
Thanks for the continued discussion here, I do think it's really good for us to think through these nuances!
My take on it is that the base title field should be the most flexible, with each particular instance that inherits from it being more opinionated. Ideally, I don't think we'd create a separate title field for media unless we have other requirements for how the title is displayed than what's included in the base title field. Or alternately, we might have postTitle in addition to mediaTitle, keeping titleField as the generic / base field.
For now, I'm pretty sold on enabling hiding on this field, so I've added a changelog entry for it. As we explore follow-ups to tackle the UX issues with hiding fields, I'm very happy to revisit this, though!
Once this PR lands, let's keep the discussion going on the linked issue: #71078
Little thing (default value is true):
In this case, due to the discussion surrounding hiding, I actually prefer keeping the value explicit here so that if someone looks to make a change to it, they'll see that it was added here intentionally.
There was a problem hiding this comment.
This is working well apart from the scenario Dan mentioned:
When using the story I note that it's now possible to hide absolutely everything and items become completely invisible. I know this was discussed already, but I feel like that's not a great situation, I don't know if there was a conclusion to the discussion:
Could we maybe ensure the title always displays if everything else is hidden?
|
Thanks for the great feedback, everyone! I think I've implemented most of the feedback, and I've added a couple of minimal tests to cover tabbing to the checkbox and actions, but let me know if that's not what you had in mind. I found writing the tests a little fiddly, so not sure if they're quite the right way to go about it.
Agreed, and it's being discussed over in #71078 (comment). I was keen to try to keep dealing with that separate from this change as I wasn't sure how complex it would be to implement, but I do understand if folks think it's a blocker to landing this one!
Yeah, I might not get to fixing that up today, but what I'd like to (somehow) try, is adding logic to the Appearance area so that you actually can't hide every property. I.e. disabling the In the meantime, I'll leave the Story in this PR for now, but will remove it before this PR eventually lands. Thanks again, all! |
|
Tiny update: after thinking it over and hacking around, I think we should resolve the issue of folks accidentally hiding all fields within this PR. I've got something working locally that looks like this: 2025-08-29.14.54.01.mp4Basically, for each of the "locked" fields (title, preview/media, description) if it's the last visible field, we treat is as locked. It feels pretty good to me so far. I'll likely run out of time to commit the code change as I've still got to handle previews properly, but I think it's promising. I'll continue on with it next week! |
Commented in the other thread, resharing for visibility #71078 (comment) |
andrewserong
force-pushed
the
try/titleless-dataviews-grid-with-hover-actions
branch
from
f89aaa2 to
abd28bd
Compare
| // Use fullscreen layout and a wrapper div with padding to resolve conflicts | ||
| // between Ariakit's Dialog (usePreventBodyScroll) and Storybook's body padding | ||
| // (sb-main-padding class). This ensures consistent layout in DataViews stories | ||
| // when clicking actions menus. Without this the padding on the body will jump. | ||
| parameters: { | ||
| layout: 'fullscreen', | ||
| }, | ||
| decorators: [ | ||
| ( Story ) => ( | ||
| <div style={ { padding: '1rem' } }> | ||
| <Story /> | ||
| </div> | ||
| ), | ||
| ], |
There was a problem hiding this comment.
While this PR doesn't add any additional stories anymore, I'm leaving this change in, as it helps make things more consistent when interacting with the actions popovers within any of the DataViews stories.
Thanks for sharing the thoughts — it sounds like there's a few different ideas to try for dealing with the issues of folks hiding different combinations of fields. I'm still pretty sold on the idea of locking the last remaining visible locked field as in the earlier comment #71369 (comment) as I think it solves a number of issues while enabling the title field's visibility to be toggled, but that doesn't preclude also investigating other options. In terms of the issue of whether or not we enable hiding for the title field itself, I think it's necessary because we need the title field to technically be present in order to provide accessible labels, so I don't think it's viable to exclude it altogether from Media / Grid views. As the discussion on what to do about edge cases where different fields aren't present (or hidden) is ongoing over in #71078 (comment), what do folks think about landing this PR in roughly the shape it's currently in, and looking into how we improve the UX in follow-ups? |
Maybe folks can educate me, but I'm reading two separate issues:
Seems to me like they can be treated as different scenarios from a UX perspective. To confirm, this PR only affects the Grid layout, right? I guess I'm really asking, what cannot be done later in a follow up, or what would this PR negatively affect, if it were merged? |
This PR affects the Grid Layout, and the generic
I don't believe this PR will have any negative impact within Gutenberg, but consumers that use the generic My hunch is that most of the improvements to the UX surrounding when no fields are hidden (or adding guardrails to prevent that from happening) could happily happen in follow-ups. |
On the face of it, I don't see this as a negative given appropriate fallbacks. Sounds like you have those covered. Thanks for the explainer! |
| screen.getAllByRole( 'button', { name: 'Actions' } )[ 0 ] | ||
| ).toHaveFocus(); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
Not something to address in this PR but these feel like they should be e2e tests. Could we perhaps run e2e against storybook for dataviews UI testing?
| @@ -18,7 +18,7 @@ const titleField: Field< CommonPost > = { | |||
| placeholder: __( 'No title' ), | |||
| getValue: ( { item } ) => getItemTitle( item ), | |||
| render: TitleView, | |||
| enableHiding: false, | |||
| enableHiding: true, | |||
There was a problem hiding this comment.
I think it makes sense for the base title to default to enable hiding. If fields can usually be hidden, disabling hiding is a decision which should probably be made at the consumer level. Both Pages and Templates have their own title fields which disable hiding, so nothing will change in the actual site editor.
There was a problem hiding this comment.
This PR in isolation is working well for me and doesn't introduce any issues in the gutenberg plugin. I left a couple of small comments, but they're just code quality things.
I do think it'd be good to have a pretty quick follow-up to prevent being able to hide all fields. I leave it up to you how to handle it, but the solution you showed looks good to me!
…utton when popovers are open
…ity is set correctly
andrewserong
force-pushed
the
try/titleless-dataviews-grid-with-hover-actions
branch
from
bd4b9f4 to
46ad2a2
Compare
|
Thanks for all the reviews and discussion here, folks! I'll merge this in now, and will continue the discussion surrounding the hiding UX over in #71078 and in follow-ups. |
andrewserong
deleted the
try/titleless-dataviews-grid-with-hover-actions
branch
What?
Part of #71078
Update the DataViews Grid layout to support hiding the title, while making the actions available on hover, similar to how selection works.
Why?
Discussed in #71078, but overall the goal is to be able to support grid layouts that are simple in appearance (i.e. just a grid of images) while still allowing the flexibility and control of things like actions.
How?
titlefield to be hiddenHStackfor the title when the title is presentusePreventBodyScrolland the default padding applied to the body element in Storybook.Testing Instructions
npm run storybook:devand go to the Default story: http://localhost:50240/?path=/story/dataviews-dataviews--defaulttrunkScreenshots or screencast
2025-08-28.14.30.24.mp4