[Data Views][Grid Layout] Making cmd/ctrl clicking more apparent #59292
Conversation
andrewhayward
added
[Type] Experimental
andrewhayward
requested review from
jorgefilipecosta,
ntsekouras and
youknowriad
|
Size Change: +521 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d80eae2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8009989619
|
andrewhayward
changed the title
| doc.body.addEventListener( 'keydown', listener, config ); | ||
| doc.body.addEventListener( 'keyup', listener, config ); |
There was a problem hiding this comment.
Adding keyup/keydown listeners when the mouse pointer enters the grid, and removing when the mouse pointer leaves again. The event listeners are added to the body so that the key press is captured regardless of which element has focus.
|
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. |
| gap={ 6 } | ||
| columns={ 2 } | ||
| alignment="top" | ||
| className="dataviews-view-grid" | ||
| aria-busy={ isLoading } | ||
| onMouseEnter={ pointerIsWithinBoundsHandler } |
There was a problem hiding this comment.
Curious why you added these events in Grid and not in GridItem..
There was a problem hiding this comment.
Mostly just to reduce the number of events being processed; we're manually adding and removing keyup and keydown event handlers to the body, and doing that every time the mouse pointer moves in and out of a grid item could get expensive.
The key* event handlers have to be on the body, because focus could be anywhere; putting them on each grid item would require them to have focus to function properly.
| </VStack> | ||
| </div> | ||
| { hasNoPointerEvents && ( | ||
| <div |
There was a problem hiding this comment.
This is probably here for a11y reasons(role=checkbox), right? If yes, could we move them to the dataviews-view-grid__content div?
packages/dataviews/src/style.scss
Outdated
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| cursor: cell; |
There was a problem hiding this comment.
If we go with this design, maybe we should change the cursor to a - if it's already selected? 🤔
There was a problem hiding this comment.
I don't think such a cursor exists.
There was a problem hiding this comment.
The cell cursor is only incidentally a ➕, and as @jameskoster says, there is no ➖ equivalent unfortunately.
|
Thanks for the PR @andrewhayward! I agree that this is something we can improve and would love some design feedback about it. Is a cursor enough, or an approach with a light overlay on the grid item or something else.. |
|
I don't mind the cursor, it seems to fit the purpose, and is certainly an improvement than the mixture you see currently. An alternative (or addition if we want further indication) could be to apply the |
|
Would you be able to add a visual, or clearer instructions on how to replicate this? |
Added a screen capture of the process, and altered the instructions slightly. |
We can pretty much do whatever we want at this stage. Easy enough to change the border colour, scale the grid item, etc.
|
|
Scaling could be neat but we don't have many other examples, so it may be better to stick with the |
|
Thank you for the added visual 🙏 I actually think the crosshair is not going to be a good fit here. I virtually never see the crosshair cursor used, and seeing it here would actually add to my confusion: am I doing something wrong? It's similar to selecting files in the Finder or Windows Explorer, seeing a crosshair when you select files would likely be equally jarring. To that end, if we are going to change the cursor, I'd change it to |
|
As I mentioned above, if we use the |
|
Updated the styling, and updated the preview screenshot.
|
|
What's the "not allowed" cursor meant to imply? I genuinely think we should be careful with changing the cursor too much. It feels so jarring when the system cursor changes to something you don't see that often, especially in this case it feels like it's suggesting you did something wrong. I'd personally be most comfortable with using primarily default and pointer cursors, swapping between those two, and then combining with any visual style changes in the list design itself. |
To put the card into this "unified checkbox" state, it requires an explicit key press (Cmd/Ctrl); the thinking was that it may not be immediately clear to the user why the click didn't register, and whether their key press wasn't picked up. Essentially, we're acknowledging that they might want to take the action, but that they can't.
Definitely agreed that we should be wary of how we use cursors, and I wouldn't have included this here if there was no other interaction involved; the disabled checkboxes for example are fine with a standard cursor. But as mentioned above, because the user has already started the action with the key press, it's helpful to be explicit about the fact it can't be completed in that context.
All that being said, I don't have a particularly strong opinion, and I'm happy to go with whatever we collectively think is most appropriate. Mostly just exploring options. |
|
I understand this is subjective but I actually appreciate the I'd also suggest that using Another option to try would be; use |
The problem here is that it isn't clear what the intended action is. There's nothing dangerous about you clicking a checkbox. But the default pointer suggests enough that it's not interactive. This isn't a strong opinion on my part, my main concern is that it's surprising to see that cursor because you almost never see it, anywhere, to be honest. I don't recall any other website or app I've seen that cursor on. |
Doesn't the keypress make it clear? Cmd+click is a very common pattern for selection. It feels strange to me to see the |
See also: #59188
Will need to be refactored a bit if #59279 lands first, but the concept will be the same.
What?
Makes it clearer that Cmd/Ctrl-clicking on a grid item is an available action.
Why?
It is not currently clear that grid items can be Cmd/Ctrl-clicked.
How?
Testing Instructions
➕➕Testing Instructions for Keyboard
This change does not impact keyboard interaction.
Screenshot