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 Editor: Maintain selection when multi-selection toggled #16336

Closed
wants to merge 3 commits into from
+89 −3
Closed

Block Editor: Maintain selection when multi-selection toggled #16336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50e1830
Block Editor: Maintain selection when multi-selection toggled
aduth May 30, 2019
81e04f3
Block Editor: Cancel multi-selecting state when toggling multi-selection
aduth Jul 1, 2019
0be9fea
Block Editor: Document block selection state object
aduth Jul 1, 2019
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
30 changes: 27 additions & 3 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,27 @@ export function isCaretWithinFormattedText( state = false, action ) {
}

const BLOCK_SELECTION_EMPTY_OBJECT = {};
Copy link
Member Author

Choose a reason for hiding this comment

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

A few notes on my documentation attempts here (cc @ellatrix):

  • I assumed BLOCK_SELECTION_EMPTY_OBJECT was used here to allow for some strict equality checks in selectors, though I could not find any place we actually do this. The tests also pass if its use below in BLOCK_SELECTION_INITIAL_STATE are replaced with new objects. Can you clarify what purpose this value serves?
  • And on that note, for what reason does it need to be an object? I had planned to document start and end as an object, but could only find that we reference clientId on it. Are there other properties, or could it just be replaced with a direct reference to the clientId?

Copy link
Member

Choose a reason for hiding this comment

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

Don't we do any strict equality checks in the reducer? Perhaps I removed that later on, so this wouldn't be needed any longer.

The selection object can also include the rich text identifier and caret position, if a rich text instance is selected.


/**
* Initial state object for block selection.
*
* @property {Object} start Block anchor from which the selection
* begins.
* @property {Object} end Block extent to which the selection
* ends.
* @property {boolean} isMultiSelecting Flag representing whether a multi-
* selection interaction is in progress.
* @property {boolean} isEnabled Flag representing whether multi-
* selection is currently allowed.
* @property {number?} initialPosition For a changed selection, the position
* at which the caret should be placed.
* Either null (default position) or -1
* (at the end of the block).
*
* @typedef {WPBlockSelectionState}
*
* @type {Object}
*/
const BLOCK_SELECTION_INITIAL_STATE = {
start: BLOCK_SELECTION_EMPTY_OBJECT,
end: BLOCK_SELECTION_EMPTY_OBJECT,
Expand All @@ -710,10 +731,10 @@ const BLOCK_SELECTION_INITIAL_STATE = {
/**
* Reducer returning the block selection's state.
*
* @param {Object} state Current state.
* @param {WPBlockSelectionState} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
* @return {WPBlockSelectionState} Updated state.
*/
export function blockSelection( state = BLOCK_SELECTION_INITIAL_STATE, action ) {
switch ( action.type ) {
Expand Down Expand Up @@ -809,8 +830,11 @@ export function blockSelection( state = BLOCK_SELECTION_INITIAL_STATE, action )
}
case 'TOGGLE_SELECTION':
return {
...BLOCK_SELECTION_INITIAL_STATE,
...state,
isEnabled: action.isSelectionEnabled,
isMultiSelecting: action.isSelectionEnabled ?
state.isMultiSelecting :
false,
};
case 'SELECTION_CHANGE':
return {
Expand Down
62 changes: 62 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,6 +1727,68 @@ describe( 'state', () => {
expect( state1 ).toBe( original );
} );

it( 'should maintain selection when toggling multi-selection', () => {
const original = deepFreeze( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isMultiSelecting: false,
} );

const state = blockSelection( original, {
type: 'TOGGLE_SELECTION',
isSelectionEnabled: false,
} );

expect( state ).toEqual( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isMultiSelecting: false,
isEnabled: false,
} );
} );

it( 'should cancel multi-selection when disabling multi-selection', () => {
const original = deepFreeze( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isMultiSelecting: true,
} );

const state = blockSelection( original, {
type: 'TOGGLE_SELECTION',
isSelectionEnabled: false,
} );

expect( state ).toEqual( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isEnabled: false,
isMultiSelecting: false,
} );
} );

it( 'should preserve multi-selection when enabling multi-selection', () => {
[ true, false ].forEach( ( isMultiSelecting ) => {
const original = deepFreeze( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isMultiSelecting,
} );

const state = blockSelection( original, {
type: 'TOGGLE_SELECTION',
isSelectionEnabled: true,
} );

expect( state ).toEqual( {
start: { clientId: 'ribs' },
end: { clientId: 'ribs' },
isEnabled: true,
isMultiSelecting,
} );
} );
} );

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

Expand Down