-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Dataviews: Add ctrl/cmd + click multiselection support to table layout when clicking rows #70891
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
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. |
Size Change: +30 B (0%) Total Size: 1.89 MB
ℹ️ View Unchanged
|
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.
Testing well for me. I think this is a good change, but will defer to DataView experts.
? selection.filter( ( itemId ) => id !== itemId ) | ||
: [ id ] | ||
); | ||
if ( isAppleOS() ? event.metaKey : event.ctrlKey ) { |
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.
Is the isAppleOS
check more accurate than ( event.metaKey || event.ctrlKey )
?
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.
I probably could've said a bit more about this in the PR description:
( event.metaKey || event.ctrlKey )
does work ok on Mac OS, if you try ctrl + click, it opens the contextual menu rather than multiselecting.
On Windows though you can use the 'Windows' key + click to multiselect, which isn't normal, usually on Windows it's ctrl + click to multiselect, so this is what it's fixing.
I haven't been able to test Linux, but I believe ctrl + click is normal there as well (I looked up some docs that mention it) so it should work correctly there as well.
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.
On Windows though you can use the 'Windows' key + click to multiselect, which isn't normal, usually on Windows it's ctrl + click to multiselect, so this is what it's fixing.
Got it, thanks for the explainer.
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.
A note on consecutive multi-selection (e.g. using shift key) - I did consider trying to tackle it, but it's quite a bit more complex. The selection code needs to be aware of the range of items, which currently it isn't, it only knows about the clicked item. I'm also not sure if it should work across pagination.
Would be nice to have at some point! I wouldn't expect it to work across pagination any more than the select all option does (it doesn't) 😄
@@ -117,7 +118,7 @@ function GridItem< Item >( { | |||
'is-selected': hasBulkAction && isSelected, | |||
} ) } | |||
onClickCapture={ ( event ) => { | |||
if ( event.ctrlKey || event.metaKey ) { | |||
if ( isAppleOS() ? event.metaKey : event.ctrlKey ) { | |||
event.stopPropagation(); |
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.
Was this not working as expected? I can't tell the difference between trunk and this branch on the grid layout.
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.
As mentioned above, it wasn't working as expected on Windows - #70891 (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.
From my perspective this is a good change and brings consistency to the views.
Kapture.2025-07-29.at.14.17.01.mp4
Adding the keycodes package isn't too bloaty and might be needed anyway for bulk and potentially SHIFT key selection.
I know you were waiting on @oandregal for an opinion, but I guess we could always do a follow up if changes are required.
Just remembered I should add a change log entry, I'll follow-up with another PR. edit: #70953 |
…t when clicking rows (WordPress#70891) * Add non-consecutive multi-selection to table view * Use correct modifier key in grid view for each OS * Add keycodes dependency * Add tests for single and multiselection for table and grid * Add keycodes to tsconfig ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
What?
Closes #70721
I noticed that in the table layout there's no way to multi-select when clicking the row, users can only select a single item.
Grid does however support this, so it's an inconsistency.
A note on consecutive multi-selection (e.g. using shift key) - I did consider trying to tackle it, but it's quite a bit more complex. The selection code needs to be aware of the range of items, which currently it isn't, it only knows about the clicked item. I'm also not sure if it should work across pagination. It might require moving a bit more code around.
How?
Testing Instructions
Screenshots or screencast
Kapture.2025-07-24.at.16.59.25.mp4