-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: Support hiding the title in Grid layout, with actions available on hover #71369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0ef6433
baca492
8bb229b
87ea4a2
81c717f
17f65b3
837f60b
ef23e9f
cb192d2
f2922db
262aa0c
0a5ca80
46ad2a2
3ae7918
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -510,6 +510,92 @@ describe( 'DataViews component', () => { | |
| await user.keyboard( '{/Control}' ); | ||
| } ); | ||
|
|
||
| it( 'supports tabbing to selection and actions when title is visible', async () => { | ||
| render( | ||
| <DataViewWrapper | ||
| view={ { | ||
| ...DEFAULT_VIEW, | ||
| type: 'grid', | ||
| fields: [], | ||
| mediaField: 'image', | ||
| titleField: 'title', | ||
| } } | ||
| isItemClickable={ () => true } | ||
| actions={ actions } | ||
| /> | ||
| ); | ||
|
|
||
| // Double check that the title is being rendered. | ||
| expect( screen.getByText( data[ 0 ].title ) ).toBeInTheDocument(); | ||
|
|
||
| const viewOptionsButton = screen.getByRole( 'button', { | ||
| name: 'View options', | ||
| } ); | ||
|
|
||
| const user = userEvent.setup(); | ||
|
|
||
| // Double click to open and then close view options. This is performed | ||
| // instead of a direct .focus() so that effects have time to complete. | ||
| await user.click( viewOptionsButton ); | ||
| await user.click( viewOptionsButton ); | ||
|
|
||
| await user.tab(); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'checkbox', { name: data[ 0 ].title } ) | ||
| ).toHaveFocus(); | ||
|
|
||
| await user.tab(); | ||
|
|
||
| expect( | ||
| screen.getAllByRole( 'button', { name: 'Actions' } )[ 0 ] | ||
| ).toHaveFocus(); | ||
| } ); | ||
|
|
||
| it( 'supports tabbing to selection and actions when title is not visible', async () => { | ||
| render( | ||
| <DataViewWrapper | ||
| view={ { | ||
| ...DEFAULT_VIEW, | ||
| type: 'grid', | ||
| fields: [], | ||
| mediaField: 'image', | ||
| titleField: 'title', | ||
| showTitle: false, | ||
| } } | ||
| isItemClickable={ () => true } | ||
| actions={ actions } | ||
| /> | ||
| ); | ||
|
|
||
| // Double check that the title is not being rendered. | ||
| expect( | ||
| screen.queryByText( data[ 0 ].title ) | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| const viewOptionsButton = screen.getByRole( 'button', { | ||
| name: 'View options', | ||
| } ); | ||
|
|
||
| const user = userEvent.setup(); | ||
|
|
||
| // Double click to open and then close view options. This is performed | ||
| // instead of a direct .focus() so that effects have time to complete. | ||
| await user.click( viewOptionsButton ); | ||
| await user.click( viewOptionsButton ); | ||
| await user.tab(); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'checkbox', { name: data[ 0 ].title } ) | ||
| ).toHaveFocus(); | ||
|
|
||
| await user.tab(); | ||
|
|
||
| expect( | ||
| screen.getAllByRole( 'button', { name: 'Actions' } )[ 0 ] | ||
| ).toHaveFocus(); | ||
| } ); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| it( 'accepts an invalid previewSize and the preview size picker falls back to another size', async () => { | ||
| render( | ||
| <DataViewWrapper | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,7 +18,7 @@ const titleField: Field< CommonPost > = { | |||||
| placeholder: __( 'No title' ), | ||||||
| getValue: ( { item } ) => getItemTitle( item ), | ||||||
| render: TitleView, | ||||||
| enableHiding: false, | ||||||
| enableHiding: true, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, nice. My worry was about affecting existing screens 👍 Little thing (default value is
Suggested change
I think of Thanks to you both for the context. I now understand this change better.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
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. |
||||||
| enableGlobalSearch: true, | ||||||
| filterBy: false, | ||||||
| }; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's nice.