Add multi select #896

ellatrix · 2017-05-24T17:02:25Z

See #62.

ellatrix · 2017-05-24T17:02:46Z

More work to do there. Only adds a background.

ellatrix · 2017-05-24T17:06:22Z

@jasmussen Feel free to style as you'd like. :)

ellatrix · 2017-05-24T17:08:57Z

It's probably better to have a separate reducer for this...

ellatrix · 2017-05-24T18:19:39Z

To do:

Hide visible text selection.
Add block mover and delete button.

aduth · 2017-05-24T18:43:25Z

How will keyboard multi-selection work?

ellatrix · 2017-05-24T18:50:39Z

Good question. I thing we could add several ways. A good one would be the shortcut you normally use to select paragraph by paragraph, which is cmd+shift+arrow I believe.

aduth · 2017-05-24T18:52:16Z

I was expecting since I could click and drag across multiple blocks that I could shift+down from a paragraph into the next. Might be difficult to implement, since it's hard to know when shift+down occurs on the "last" line of a paragraph.

ellatrix · 2017-05-24T18:54:00Z

@aduth You mean once you have a selection? If there's no selection yet, it will conflict with selecting line-by-line.

ellatrix · 2017-05-24T18:56:35Z

Another thing we might want to do, in addition to drag, is a select mode, much like you have in the iOS Photos app. If it's toggled, clicking could select multiple blocks. The only awkward thing is that they'll be forced consecutive.

ellatrix · 2017-05-24T18:57:16Z

Maybe @afercia has some more ideas.

aduth · 2017-05-24T18:58:45Z

This is what I mean: Repeatedly Shift+Down, I hit the end of the text block, but never moves into multi selection, which feels disjointed from the click+drag experience: https://cloudup.com/c-8wUVJ4iIv

ellatrix · 2017-05-24T19:00:41Z

Aha! Yeah, I should be able to make that work. Again here a better selection state will be helpful. :)

afercia · 2017-05-24T21:00:04Z

If using repeatedly Shift+Down, how screen reader users would know when they've reached the end of a block? I'm not sure any kind of keyboard shortcut here would be ideal, since it will be confusing with what normally happens in a textarea. Really not sure what to do. The whole keyboard interaction for navigation and selection across blocks is a real issue, I have no great ideas so far.

jasmussen · 2017-05-25T09:26:11Z

Looks good, works pretty well, nice work! I added a slightly darker border around each selected block.

Hide visible text selection.

Can we do this by simply styling the selection rectangle the same color as the background, when you have multi-selected? Seems like it would be good to only hide the text selection, not actually remove it. We might find it annoying if you're just trying to select text inside a block, and accidentally invoke the multi block selection thing to ony have your selection undone.

In that vein it might be good to have some buffer zone or timer, so that you only actually start selecting across blocks once your mouse is, like, n pixels over the next block, and only then after half a second has passed. Something in that vein would be good to experiment with.

Right now you can't deselect. Should we make it so clicking outside, just like how you deselect text, deselects blocks too? Escape should probably also deselect.

How will keyboard multi-selection work?

Maybe ⌘A, when focus is outside of a block, selects all blocks. But maybe you can't select just a few at all, using just the keyboard.

The purpose of multi selection is to make it easy to delete multiple blocks at once, or move multiple blocks at once. You will already be able to do this using keyboard shortcuts alone — simply invoke the keyboard shortcuts for delete blocks multiple times in a row. Or tab to the block you want to delete, then delete it. Same with rearranging blocks.

That is — you can accomplish the same end result using keyboard shortcuts. It's just a slightly different flow. This is similar to how layouting apps like Keynote or Sketch work.

ellatrix · 2017-05-26T13:22:26Z

Thanks for adding the styling! I saw you design also had some spacing in between, is this something you'd like to see too?

Seems like it would be good to only hide the text selection, not actually remove it.

I suggested hiding right? :)

I'll have a look at the buffering and deselect.

ellatrix · 2017-05-27T09:19:16Z

Right now you can't deselect.

For me deselecting works by clicking on any of the blocks.

ellatrix · 2017-05-28T14:56:47Z

I've added the delete and moving buttons. Still needs some tests. Also the location of these buttons may not be ideal.

…ngle selection

youknowriad

Realy awesome job here 👏🏻

I'm seeing some minor issues:

Removing using backspace is not working (The "regular" backspace event is being handled instead.
Removing using the button don't work sometimes. I'm not sure when it works and when it doesn't
The sticky behaviour (remove button) is not working as expected.

youknowriad · 2017-05-29T14:49:17Z

editor/modes/visual-editor/block-list.js

+							onSelectionStart={ () => this.onSelectionStart( uid ) }
+							onSelectionChange={ () => this.onSelectionChange( uid ) }
+							onSelectionEnd={ this.onSelectionEnd }
+							selectedBlocks={ selectedBlocks }


Should we drop the selectedProps prop and move it to the connect of VisualEditorBlock instead?

youknowriad · 2017-05-29T14:52:31Z

editor/modes/visual-editor/block.js

@@ -229,7 +241,26 @@ class VisualEditorBlock extends wp.element.Component {
 						</div>
 					</CSSTransitionGroup>
 				}
-				<div onKeyPress={ this.maybeStartTyping }>
+				{ this.props.isFirstSelected && (


Minor: Could we destructure all the this.props.*?

Yeah. I think I forgot to remove this after I saw it clashes with the selector... Any naming conventions here?

I'm ok keeping it to avoid conflicting with the selector. but Maybe I'd rename the selector isFirstSelectedBlock

Sounds good!

youknowriad · 2017-05-29T14:56:07Z

editor/state.js

 		switch ( action.type ) {
 			case 'RESET_BLOCKS':
 				return action.blocks.map( ( { uid } ) => uid );

-			case 'INSERT_BLOCK':
+			case 'INSERT_BLOCK': {


Curious to know why do you wrap case between {}. Is this a good practice I'm missing?

Yeah I'd rather scope the const declarations to one case instead of the whole reducer. You can use {} anywhere you want to create a new block scope AFAIK.

/* ... */ { let something = 1; } /* ... */

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/block

Gotcha, Thanks for the explanation

ellatrix · 2017-05-29T16:09:12Z

@youknowriad Yep, I know the sticky positioning is not working... I'm not sure how to resolve this atm. Was thinking to do try to address this in a separate PR. I'll add the backspace shortcut.

ellatrix · 2017-05-29T17:06:39Z

Regarding sticky positioning... We could wrap the selected block in an extra div. This would also be far cleaner than adding the buttons to the first selected block. A big downside is that iframes would reload. The other solution is adding the buttons to the parent division, but we'll have to reposition it with JS.

ellatrix · 2017-05-29T17:17:40Z

Something else that just popped in my mind: overlay the selected area with a div than contains the buttons and can be clicked through. Downside here is that the width and height may easily get out of sync.

jasmussen · 2017-05-30T05:28:24Z

This feels pretty good, especially as a first PR version of this. Click outside to deselect works great, as does the text selection trick you're doing. Great work!

I can't get the mover to work.

The sticky toolbar and cross-select "buffer zone" seem like perfect candidates to address separately.

On Sticky — would it make sense to think of a "global toolbar" component, which can be invoked in situations like this? I could imagine us using this one for mobile improvements as well. That is, a sticky toolbar that sits where it needs to sit in the DOM in order to scroll the full length of the document, but is only used as a tray for buttons in certain situations?

The pattern to emulate could be that of Google Photos, where you have one toolbar when you don't have any photos selected:

... and another toolbar replacing it, when you have photos selected:

Might be nice to even copy the "8 blocks selected" pattern. This could even be the location of the mover buttons.

Thanks for adding the styling! I saw you design also had some spacing in between, is this something you'd like to see too?

Not critical, and not worth doing in this PR. The truth is right now the blocks are flush next to each other, so that's why there isn't that space. However if we remove one px of vertical padding inside the blocks, and make that margin between them instead, it should address that. But because of collapsing margins and such, worth doing separately.

ellatrix · 2017-05-30T09:38:09Z

I agree that we should iterate on the rest of the issues. I'd like to get it merged ASAP in this state. :) The block movers were working for me be let me double check. :)

ellatrix · 2017-05-30T09:42:42Z

Huh, I broke them...

ellatrix · 2017-05-30T09:52:52Z

@jasmussen Actually no, they work fine here. I'm not sure why it's not working for you.

ellatrix · 2017-05-30T09:59:46Z

Sorry, it's the last commit, I see it now. Got confused between what I had checked out. :/

jasmussen · 2017-05-30T12:07:22Z

Moving seems to work now! It doesn't scroll you to the new location like moving an individual block does, though. Is this fixable? If yes, fine to do in a separate PR.

ellatrix · 2017-05-30T12:08:50Z

Yeah I knew it wouldn't scroll. I'll quickly have a look how much it takes to make it work.

jasmussen · 2017-05-30T12:32:40Z

Impressive! Moving works great now! 💖

ellatrix · 2017-05-30T13:47:28Z

Any technical concerns? Otherwise I'll merge in the next few hours.

aduth · 2017-05-31T15:02:21Z

components/button/index.js

+	}
+
+	componentDidMount() {
+		if ( this.props.focus ) {


Could you explain the issues with focus you were seeing? Do we need to monitor this prop if it changes during the lifetime of the component?

Could you explain the issues with focus you were seeing? Do we need to monitor this prop if it changes during the lifetime of the component?

Circling back to this. I encountered the Toolbar focus prop when working on some unrelated refactoring and had no idea what its purpose is. I still don't, and could use some clarification as to this unanswered question.

aduth · 2017-05-31T15:03:19Z

components/icon-button/index.js

@@ -10,11 +10,11 @@ import './style.scss';
 import Button from '../button';
 import Dashicon from '../dashicon';

-function IconButton( { icon, children, label, className, ...additionalProps } ) {
+function IconButton( { icon, children, label, className, focus, ...additionalProps } ) {


focus would be passed to the underlying Button without being explicitly handled here, based on the behavior of additionalProps spread.

BE-Webdesign · 2017-06-01T14:52:32Z

components/toolbar/index.js

@@ -30,6 +30,7 @@ function Toolbar( { controls } ) {
 						'is-active': control.isActive,
 					} ) }
 					aria-pressed={ control.isActive }
+					focus={ focus && ! index }


This line is causing some trouble for me in tests. I am unsure as to what is happening here. Where is focus coming from? Is this supposed to be passed into the component as a prop? If that is the case, we will need to add focus to the function signature at the top. Toolbar( { controls, focus } ).

You're right. This was causing a bug too. Thanks for catching it!

ellatrix mentioned this pull request May 24, 2017

Editable Strategy #898

Closed

ellatrix force-pushed the add/multi-select branch 2 times, most recently from 8b3e6ff to aa092eb Compare May 27, 2017 09:07

ellatrix force-pushed the add/multi-select branch from 6d881c9 to 1637cac Compare May 28, 2017 14:55

ellatrix and others added 7 commits May 29, 2017 13:23

Add multi select

a687b81

Add darker blue border on selection rectangles.

1cb0e46

Hide selection backgroud on multi selection

4ef611a

Separate reducer for multi selection: fixes setting state back for si…

01d124a

…ngle selection

Deselect blocks on click outside

cd04a65

Add buttons

ec368f5

Add tests

91b577c

ellatrix force-pushed the add/multi-select branch from 1637cac to 91b577c Compare May 29, 2017 11:58

ellatrix requested a review from youknowriad May 29, 2017 12:00

ellatrix requested a review from aduth May 29, 2017 12:00

ellatrix added this to the May 29 to Jun 2 milestone May 29, 2017

mtias added [Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels. labels May 29, 2017

youknowriad reviewed May 29, 2017

View reviewed changes

Code polish

d489fce

Add esc and backspace

336eade

Fix focus issue on the buttons

236bf6f

Keep scroll position when moving the selected blocks

6ef9c81

ellatrix requested a review from youknowriad May 30, 2017 12:35

ellatrix merged commit a14d015 into master May 30, 2017

ellatrix deleted the add/multi-select branch May 30, 2017 16:33

This was referenced May 31, 2017

Multi Select: Add buffer zone #950

Closed

Multi Select: Better sticky toolbar #951

Closed

aduth reviewed May 31, 2017

View reviewed changes

BE-Webdesign reviewed Jun 1, 2017

View reviewed changes

youknowriad mentioned this pull request Jun 2, 2017

Inserter: Fix the insertion point #994

Merged

aduth mentioned this pull request Oct 27, 2017

Components: Remove unused focus prop from Button #3201

Merged

Add multi select #896

Add multi select #896

Conversation

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

aduth commented May 24, 2017

ellatrix commented May 24, 2017 • edited Loading

aduth commented May 24, 2017

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

ellatrix commented May 24, 2017

aduth commented May 24, 2017

ellatrix commented May 24, 2017

afercia commented May 24, 2017

jasmussen commented May 25, 2017 • edited Loading

ellatrix commented May 26, 2017 • edited Loading

ellatrix commented May 27, 2017 • edited Loading

ellatrix commented May 28, 2017 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented May 29, 2017 • edited Loading

ellatrix commented May 29, 2017 • edited Loading

ellatrix commented May 29, 2017

jasmussen commented May 30, 2017

ellatrix commented May 30, 2017

ellatrix commented May 30, 2017

ellatrix commented May 30, 2017

ellatrix commented May 30, 2017

jasmussen commented May 30, 2017

ellatrix commented May 30, 2017

jasmussen commented May 30, 2017

ellatrix commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented May 24, 2017 •

edited

Loading

jasmussen commented May 25, 2017 •

edited

Loading

ellatrix commented May 26, 2017 •

edited

Loading

ellatrix commented May 27, 2017 •

edited

Loading

ellatrix commented May 28, 2017 •

edited

Loading

youknowriad May 29, 2017 •

edited

Loading

ellatrix May 29, 2017 •

edited

Loading

ellatrix commented May 29, 2017 •

edited

Loading

ellatrix commented May 29, 2017 •

edited

Loading