Skip to content
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

Multi select on shift+click #1965

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Multi select on shift+click #1965

merged 1 commit into from
Oct 13, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 20, 2017

Not to be confused with shift+arrow keys, which will be addressed in a different PR.

With this PR you can multi-select block by pressing shift and then clicking. At the moment you can only select by clicking and dragging while holding the click. See #2990.

To test:

  • Select a block.
  • Press shift and click on another block. There should be a multi-selection with both these blocks and all blocks in between.
  • Click again on a block further away from the first block you selected. Multi-selection should now have extended to the last clicked block.
  • Now click on a block closer to the one you first selected, inside the multi-selection. Multi-selection should now have shrunk form the initial selected block to the last clicked block.
  • Verify that shift+click still works in an editable area.

@ellatrix ellatrix self-assigned this Jul 20, 2017
@ellatrix ellatrix requested a review from aduth July 20, 2017 15:45
@ellatrix ellatrix changed the title Multi select on shift+pointer Multi select on shift+click Jul 20, 2017
@ellatrix ellatrix mentioned this pull request Jul 20, 2017
20 tasks
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One UX change to confirm is: When no blocks are selected, and I shift-click a block, nothing happens. Previous to these changes, the text in the block would still highlight (using default browser behavior, I assume).

Also generally don't feel that this addresses usability flows noted in #179. I can describe more there, but I expect to be able to extend partial selection between paragraphs and either backspace to collapse or start typing to replace. Multi-select doesn't allow for this, really only entire delete and rearrangement.

@ellatrix ellatrix force-pushed the try/shift-multi-select branch 2 times, most recently from 32ccbf8 to 7f96c8c Compare October 12, 2017 15:14
@ellatrix ellatrix requested a review from mcsf October 12, 2017 15:14
@ellatrix ellatrix added the [Type] Enhancement A suggestion for improvement. label Oct 12, 2017
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me, and the logic is clear. Nice. :)

@@ -213,6 +227,7 @@ class VisualEditorBlockList extends Component {
uid={ uid }
blockRef={ ( ref ) => this.setBlockRef( ref, uid ) }
onSelectionStart={ () => this.onSelectionStart( uid ) }
onSelectionExtension={ () => this.onSelectionExtension( uid ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's beside the object of this PR, but I'll share the note: we probably shouldn't be binding (and re-binding) these methods before passing them to VisualEditorBlock instances — they're all grown up and can do that themselves in the constructor, or just pass their uid explicitly. :)

this.props.onSelect();
if ( event.shiftKey ) {
if ( ! this.props.isSelected ) {
this.props.onSelectionExtension();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a more descriptive name for onSelectionExtension, but I don't know what that could be. :) I know they are controversial, but I like maybe-prefixed names, like maybeMultiSelect, but that clashes with the on- framing. onShiftSelection?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe seems strange because something always happens. I don't mind onShiftSelection.

@ellatrix ellatrix force-pushed the try/shift-multi-select branch from 7f96c8c to e2a6fa2 Compare October 13, 2017 14:10
@codecov
Copy link

codecov bot commented Oct 13, 2017

Codecov Report

Merging #1965 into master will increase coverage by 0.45%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1965      +/-   ##
==========================================
+ Coverage    33.4%   33.85%   +0.45%     
==========================================
  Files         197      197              
  Lines        5778     6037     +259     
  Branches     1023     1098      +75     
==========================================
+ Hits         1930     2044     +114     
- Misses       3247     3348     +101     
- Partials      601      645      +44
Impacted Files Coverage Δ
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/gallery/block.js 9.75% <0%> (-1.36%) ⬇️
blocks/library/table/table-block.js 7.4% <0%> (-0.6%) ⬇️
editor/block-settings-menu/content.js 0% <0%> (ø) ⬆️
editor/block-toolbar/index.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
editor/block-switcher/index.js 0% <0%> (ø) ⬆️
utils/focus/tabbable.js 100% <0%> (ø) ⬆️
blocks/library/image/block.js 0% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65b43ca...e2a6fa2. Read the comment docs.

@ellatrix ellatrix merged commit 2328828 into master Oct 13, 2017
@ellatrix ellatrix deleted the try/shift-multi-select branch October 13, 2017 14:30
@gziolo
Copy link
Member

gziolo commented Oct 16, 2017

That was fast, thanks for implementing this one 😄

Edit: I see now, it was opened for weeks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants