[Data Views] Grid layout keyboard/mouse interaction changes #58802
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewhayward
added
Needs Design Feedback
andrewhayward
requested review from
jameskoster,
jorgefilipecosta,
ntsekouras and
a team
|
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: +2.98 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
This is excellent! A couple of quick thoughts on the mouse interactions;
|
I think that's an artefact of the next point, because yes it should be consistent.
This was the intent, but clearly something was missed. Will fix that.
In my head it was to maintain the consistency of using Cmd/Ctrl + |
ntsekouras
reviewed
Feb 9, 2024
|
|
||
| .dataviews-view-grid__title-actions { | ||
| padding: $grid-unit-05 $grid-unit $grid-unit-05 $grid-unit-05; | ||
| } | ||
|
|
||
| .dataviews-view-grid__primary-field { | ||
| min-height: $grid-unit-50; | ||
|
|
||
| a, | ||
| button.components-button.is-link { |
There was a problem hiding this comment.
In general we should avoid overriding components styles like that. Additionally they seem too specific that a consumer could use a Button component. What these styles do?
ntsekouras
reviewed
Feb 9, 2024
| const siblings = Array.from( element.parentElement.children ); | ||
| const index = siblings.indexOf( element ) + 1; | ||
|
|
||
| return `${ parent } > ${ element.nodeName }:nth-child(${ index })`; |
There was a problem hiding this comment.
I'm still reviewing and there are lots of things going on here, so sorry if some of my questions are obvious or wrong, but I want to understand fully what's going on :).
Why do we need create a selector and then query the doc to focus(which is expensive)? Couldn't we do that by keeping track of the ref?
ntsekouras
reviewed
Feb 9, 2024
|
|
||
| const { filters, page, perPage, search, sort } = view; | ||
| const ids = useMemo( | ||
| () => JSON.stringify( usedData.map( ( { id } ) => id ) ), |
There was a problem hiding this comment.
We should use the getItemId API for this.
ntsekouras
reviewed
Feb 9, 2024
| () => JSON.stringify( usedData.map( ( { id } ) => id ) ), | ||
| [ usedData ] | ||
| ); | ||
| const key = useMemo( |
There was a problem hiding this comment.
Why we need to add the key here?
ntsekouras
reviewed
Feb 9, 2024
| eventControllerRef.current = null; | ||
| } ); | ||
| const config = { capture: true, signal: controller.signal }; | ||
| doc.body.addEventListener( 'keydown', listener, config ); |
There was a problem hiding this comment.
Should we add the events in the node we want instead of the body? Why do we need it like this?
ntsekouras
reviewed
Feb 9, 2024
| ] | ||
| ); | ||
|
|
||
| const mediaEventHandlers = { |
There was a problem hiding this comment.
I'm not sure we should be that opinionated here. It happens right now in our use case to have the media and primary field do the same, but that's not necessarily something everyone wants.
ntsekouras
reviewed
Feb 9, 2024
There was a problem hiding this comment.
That's a great start @andrewhayward , thank you!!
I've left some initial comments, but there are a lot to grasp, so it will need more rounds of reviews.
Some issues I observed currently when pressing tab inside an item:
- Focuses non interactive elements, which seemed weird to me. What’s the reason for that?
- If you keep pressing
tabfocus is transferred to the body after the last field.
Some first thoughts would be that this PR maybe tries to do too much at once, and it adds a lot of complexity. Do you think we can split it in smaller PRs? It would be easier to review and make better decisions.
ntsekouras
reviewed
Feb 9, 2024
| id={ id } | ||
| store={ store } | ||
| ref={ gridRef } | ||
| onMouseEnter={ ( { metaKey, ctrlKey } ) => { |
There was a problem hiding this comment.
Can you share a few words about why we need this?
Closing this in favour of #59188, which breaks down the various issues and allows for easier reviews. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Improves keyboard navigation in grid views, and enables range selection with mouse input.
Related: #55083
Why?
Grid views are currently difficult to navigate.
How?
The grid view has been restructured using
Compositecomponents, with additional event handlers added to enable improved keyboard/mouse interaction.When focus is on a grid cell
When focus is inside a grid cell
Mouse
Clickon media/primary field activates primary field action, if possibleClicktoggles item selectionClicktoggles range to previous selectionTesting Instructions
Screenshots or screencast
grid.keyboard.interaction.mov
grid.mouse.multiselection.mov