-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViewsPicker Grid layout: Support hiding the title #71865
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
DataViewsPicker Grid layout: Support hiding the title #71865
Conversation
|
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. |
talldan
left a comment
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.
LGTM!
| className="dataviews-view-picker-grid__title-actions" | ||
| > | ||
| <div className="dataviews-view-picker-grid__title-field dataviews-title-field"> | ||
| { renderedTitleField } |
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.
Actually, I approved too quickly. A problem is that the items are all now unlabelled when using a screenreader if the visible label is hidden.
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.
Are we handling that in the original grid view? If not it might be worth fixing for both?
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.
It looks like the checkbox gets a proper label, but the click wrapper around the image is missing a label.
I think this is the relevant code:
gutenberg/packages/dataviews/src/dataviews-layouts/grid/index.tsx
Lines 117 to 130 in 9f20f50
| if ( isItemClickable( item ) && onClickItem ) { | |
| if ( renderedTitleField ) { | |
| mediaA11yProps = { | |
| 'aria-labelledby': `dataviews-view-grid__title-field-${ instanceId }`, | |
| }; | |
| titleA11yProps = { | |
| id: `dataviews-view-grid__title-field-${ instanceId }`, | |
| }; | |
| } else { | |
| mediaA11yProps = { | |
| 'aria-label': __( 'Navigate to item' ), | |
| }; | |
| } | |
| } |
Having said that, in the story the image click wrappers are missing labels even when there's a visible title, so not sure what's going on there. I guess that code is a bit buggy.
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 for flagging! I think the picker might want a slightly different approach to the standard Grid... a natural place for the label to go in the picker could be to add an aria-label to the Composite.Item if the title is hidden. I'll have a play around with that.
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.
Updated in de85bbf, seems to be working pretty well in my testing using macOS voiceover 👍
|
Flaky tests detected in de85bbf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17996866345
|
| aria-label={ | ||
| titleField | ||
| ? titleField.getValue( { item } ) || __( '(no title)' ) | ||
| : undefined | ||
| } |
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.
Just explaining the use of a ternary here and falling back to undefined. For usage where a titleField itself is not provided, falling back to undefined will allow voice over to read from text within the component.
For example, if someone uses the DataViews and does not provide a title field, but does provide a description field, then the description field will be read out.
So the approach here uses a best-effort to provide a title as the label if a title field is set, otherwise it defers to the screen reader utility to make do with what's available.
|
Alrighty, I think it's working well now in VoiceOver on macOS, so this should be ready for another review. |
tellthemachines
left a comment
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.
LGTM! Tested storybook on VoiceOver with Safari and Firefox and it's working as expected.
|
Thanks for confirming @tellthemachines, and for the reviews, folks! 🙇 |
What?
Follows:
In the DataViewsPicker grid layout, if the title is not being shown, then hide the space reserved for the title.
Why?
As in #71369 which dealt with the DataViews grid layout, when the title is hidden, we should remove the space where the title would have been displayed, so that the gap between grid items is consistent horizontally and vertically.
How?
Add a simple
showTitlecheck before displaying the area where the title appears.Testing Instructions
npm run storybook:devand navigate to the DataViewsPicker storyScreenshots or screencast
It should still look fine with the title displayed: