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.

Sign up for GitHub

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

Block List: Start multi-selection tracking on mousemove #4585

Merged
merged 2 commits into from
Jan 19, 2018
Merged

Block List: Start multi-selection tracking on mousemove #4585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b217baa
Block List: Start multi-selection tracking on mousemove
aduth Jan 18, 2018
6d6231e
State: Abort state update if multi-select action has no effect
aduth Jan 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions editor/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
getMultiSelectedBlockUids,
getSelectedBlock,
isSelectionEnabled,
isMultiSelecting,
} from '../../store/selectors';
import { startMultiSelect, stopMultiSelect, multiSelect, selectBlock } from '../../store/actions';
import { documentHasSelection } from '../../utils/dom';
Expand Down Expand Up @@ -100,7 +101,21 @@ class BlockList extends Component {
}
}

/**
* Handles a pointer move event to update the extent of the current cursor
* multi-selection.
*
* @param {MouseEvent} event A mousemove event object.
*
* @returns {void}
*/
onPointerMove( { clientY } ) {
// We don't start multi-selection until the mouse starts moving, so as
// to avoid dispatching multi-selection actions on an in-place click.
if ( ! this.props.isMultiSelecting ) {
Copy link
Member

@ellatrix ellatrix Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe a stupid question, but could this be built in the reducer if it should always be used like this? Same for stopping the selection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a stupid question, but could this be built in the reducer if it should always be used like this? Same for stopping the selection.

Can you clarify what you mean? I have two interpretations:

  • Should the state.blockSelection.isMultiSelecting flag be automatically assigned to true when MULTI_SELECT is dispatched, vs. an explicit START_MULTI_SELECT action?
    • Perhaps, if we're okay with the fact that isMultiSelecting is not reflected immediately upon starting to drag the mouse, but only after the drag extends across at least one additional block
    • I'm also not sure how it would be possible to infer the end of a multi-selection via this behavior
  • Handle the check to only change the value of isMultiSelecting if it's not already true, from the reducer itself, rather than in the component

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean the second point. Sounds good to me then. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added 6d6231e to cover our bases 😄

this.props.onStartMultiSelect();
}

const boundaries = this.nodes[ this.selectionAtStart ].getBoundingClientRect();
const y = clientY - boundaries.top;
const key = findLast( this.coordMapKeys, ( coordY ) => coordY < y );
Expand Down Expand Up @@ -138,6 +153,14 @@ class BlockList extends Component {
}
}

/**
* Binds event handlers to the document for tracking a pending multi-select
* in response to a mousedown event occurring in a rendered block.
*
* @param {string} uid UID of the block where mousedown occurred.
*
* @returns {void}
*/
onSelectionStart( uid ) {
if ( ! this.props.isSelectionEnabled ) {
return;
Expand All @@ -161,8 +184,6 @@ class BlockList extends Component {
// Capture scroll on all elements.
window.addEventListener( 'scroll', this.onScroll, true );
window.addEventListener( 'mouseup', this.onSelectionEnd );

this.props.onStartMultiSelect();
}

onSelectionChange( uid ) {
Expand All @@ -183,6 +204,11 @@ class BlockList extends Component {
}
}

/**
* Handles a mouseup event to end the current cursor multi-selection.
*
* @returns {void}
*/
onSelectionEnd() {
// Cancel throttled calls.
this.onPointerMove.cancel();
Expand All @@ -195,7 +221,11 @@ class BlockList extends Component {
window.removeEventListener( 'scroll', this.onScroll, true );
window.removeEventListener( 'mouseup', this.onSelectionEnd );

this.props.onStopMultiSelect();
// We may or may not be in a multi-selection when mouseup occurs (e.g.
// an in-place mouse click), so only trigger stop if multi-selecting.
if ( this.props.isMultiSelecting ) {
this.props.onStopMultiSelect();
}
}

onShiftSelection( uid ) {
Expand Down Expand Up @@ -244,6 +274,7 @@ export default connect(
multiSelectedBlockUids: getMultiSelectedBlockUids( state ),
selectedBlock: getSelectedBlock( state ),
isSelectionEnabled: isSelectionEnabled( state ),
isMultiSelecting: isMultiSelecting( state ),
} ),
( dispatch ) => ( {
onStartMultiSelect() {
Expand Down
16 changes: 15 additions & 1 deletion editor/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,11 @@ export function blockSelection( state = {
}, action ) {
switch ( action.type ) {
case 'CLEAR_SELECTED_BLOCK':
if ( state.start === null && state.end === null &&
state.focus === null && ! state.isMultiSelecting ) {
return state;
}

return {
...state,
start: null,
Expand All @@ -383,15 +388,24 @@ export function blockSelection( state = {
isMultiSelecting: false,
};
case 'START_MULTI_SELECT':
if ( state.isMultiSelecting ) {
return state;
}

return {
...state,
isMultiSelecting: true,
};
case 'STOP_MULTI_SELECT':
const nextFocus = state.start === state.end ? state.focus : null;
if ( ! state.isMultiSelecting && nextFocus === state.focus ) {
return state;
}

return {
...state,
isMultiSelecting: false,
focus: state.start === state.end ? state.focus : null,
focus: nextFocus,
};
case 'MULTI_SELECT':
return {
Expand Down
34 changes: 33 additions & 1 deletion editor/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,15 @@ describe( 'state', () => {
expect( state ).toEqual( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: true } );
} );

it( 'should return same reference if already multi-selecting', () => {
const original = deepFreeze( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
type: 'START_MULTI_SELECT',
} );

expect( state ).toBe( original );
} );

it( 'should end multi selection with selection', () => {
const original = deepFreeze( { start: 'ribs', end: 'chicken', focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
Expand All @@ -811,6 +820,15 @@ describe( 'state', () => {
expect( state ).toEqual( { start: 'ribs', end: 'chicken', focus: null, isMultiSelecting: false } );
} );

it( 'should return same reference if already ended multi-selecting', () => {
const original = deepFreeze( { start: 'ribs', end: 'chicken', focus: null, isMultiSelecting: false } );
const state = blockSelection( original, {
type: 'STOP_MULTI_SELECT',
} );

expect( state ).toBe( original );
} );

it( 'should end multi selection without selection', () => {
const original = deepFreeze( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
Expand All @@ -831,14 +849,28 @@ describe( 'state', () => {
expect( state1 ).toBe( original );
} );

it( 'should unset multi selection and select inserted block', () => {
it( 'should unset multi selection', () => {
const original = deepFreeze( { start: 'ribs', end: 'chicken' } );

const state1 = blockSelection( original, {
type: 'CLEAR_SELECTED_BLOCK',
} );

expect( state1 ).toEqual( { start: null, end: null, focus: null, isMultiSelecting: false } );
} );

it( 'should return same reference if clearing selection but no selection', () => {
const original = deepFreeze( { start: null, end: null, focus: null, isMultiSelecting: false } );

const state1 = blockSelection( original, {
type: 'CLEAR_SELECTED_BLOCK',
} );

expect( state1 ).toBe( original );
} );

it( 'should select inserted block', () => {
const original = deepFreeze( { start: 'ribs', end: 'chicken' } );

const state3 = blockSelection( original, {
type: 'INSERT_BLOCKS',
Expand Down