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

Jump to bottom

Block Editor: Simplify Selection Reducer #17467

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Block Editor: Simplify Selection Reducer #17467

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fb8d84
Block Editor: Simplify Selection Reducer
ellatrix Sep 17, 2019
56c281c
Test selectionStart and selectionEnd reducers instead of internal sel…
ellatrix Sep 25, 2019
243e245
Add comment when reducer resets state by default
ellatrix Sep 25, 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
2 changes: 1 addition & 1 deletion docs/designers-developers/developers/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ _Returns_
Returns true if the block corresponding to the specified client ID is
currently selected but isn't the last of the selected blocks. Here "last"
refers to the block sequence in the document, _not_ the sequence of
multi-selection, which is why `state.blockSelection.end` isn't used.
multi-selection, which is why `state.selectionEnd` isn't used.

_Parameters_

Expand Down
231 changes: 126 additions & 105 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 @@ -922,161 +922,178 @@ export function isCaretWithinFormattedText( state = false, action ) {
return state;
}

const BLOCK_SELECTION_EMPTY_OBJECT = {};
const BLOCK_SELECTION_INITIAL_STATE = {
start: BLOCK_SELECTION_EMPTY_OBJECT,
end: BLOCK_SELECTION_EMPTY_OBJECT,
isMultiSelecting: false,
isEnabled: true,
initialPosition: null,
};

/**
* Reducer returning the block selection's state.
* Internal helper reducer for selectionStart and selectionEnd. Can hold a block
* selection, represented by an object with property clientId.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function blockSelection( state = BLOCK_SELECTION_INITIAL_STATE, action ) {
function selection( state = {}, action ) {
switch ( action.type ) {
case 'CLEAR_SELECTED_BLOCK':
if ( ! state.start || ! state.start.clientId ) {
return state;
}

return {
...state,
start: BLOCK_SELECTION_EMPTY_OBJECT,
end: BLOCK_SELECTION_EMPTY_OBJECT,
isMultiSelecting: false,
initialPosition: null,
};
case 'START_MULTI_SELECT':
if ( state.isMultiSelecting ) {
return state;
}

return {
...state,
isMultiSelecting: true,
initialPosition: null,
};
case 'STOP_MULTI_SELECT':
if ( ! state.isMultiSelecting ) {
return state;
case 'CLEAR_SELECTED_BLOCK': {
if ( state.clientId ) {
return {};
}

return {
...state,
isMultiSelecting: false,
initialPosition: null,
};
case 'MULTI_SELECT':
return {
...state,
isMultiSelecting: state.isMultiSelecting,
start: { clientId: action.start },
end: { clientId: action.end },
};
return state;
}
case 'SELECT_BLOCK':
if (
action.clientId === state.start.clientId &&
action.clientId === state.end.clientId
) {
if ( action.clientId === state.clientId ) {
return state;
}

return {
...state,
initialPosition: action.initialPosition,
start: { clientId: action.clientId },
end: { clientId: action.clientId },
};
return { clientId: action.clientId };
case 'REPLACE_INNER_BLOCKS': // REPLACE_INNER_BLOCKS and INSERT_BLOCKS should follow the same logic.
case 'INSERT_BLOCKS': {
if ( action.updateSelection ) {
return {
...state,
start: { clientId: action.blocks[ 0 ].clientId },
end: { clientId: action.blocks[ 0 ].clientId },
};
if ( ! action.updateSelection ) {
return state;
}

return state;
return { clientId: action.blocks[ 0 ].clientId };
}
case 'REMOVE_BLOCKS':
if (
! action.clientIds ||
! action.clientIds.length ||
action.clientIds.indexOf( state.start.clientId ) === -1
action.clientIds.indexOf( state.clientId ) === -1
) {
return state;
}

return {
...state,
start: BLOCK_SELECTION_EMPTY_OBJECT,
end: BLOCK_SELECTION_EMPTY_OBJECT,
isMultiSelecting: false,
initialPosition: null,
};
return {};
case 'REPLACE_BLOCKS': {
if ( action.clientIds.indexOf( state.start.clientId ) === -1 ) {
if ( action.clientIds.indexOf( state.clientId ) === -1 ) {
return state;
}

const indexToSelect = action.indexToSelect || action.blocks.length - 1;
const blockToSelect = action.blocks[ indexToSelect ];

if ( ! blockToSelect ) {
return {
...state,
start: BLOCK_SELECTION_EMPTY_OBJECT,
end: BLOCK_SELECTION_EMPTY_OBJECT,
isMultiSelecting: false,
initialPosition: null,
};
return {};
}

if (
blockToSelect.clientId === state.start.clientId &&
blockToSelect.clientId === state.end.clientId
) {
if ( blockToSelect.clientId === state.clientId ) {
return state;
}

return {
...state,
start: { clientId: blockToSelect.clientId },
end: { clientId: blockToSelect.clientId },
};
return { clientId: blockToSelect.clientId };
}
case 'TOGGLE_SELECTION':
}

return state;
}

/**
* Reducer returning the block selection's start.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function selectionStart( state = {}, action ) {
switch ( action.type ) {
case 'SELECTION_CHANGE':
return {
...state,
isEnabled: action.isSelectionEnabled,
clientId: action.clientId,
attributeKey: action.attributeKey,
offset: action.startOffset,
};
case 'RESET_SELECTION':
return action.selectionStart;
case 'MULTI_SELECT':
return { clientId: action.start };
}

return selection( state, action );
}

/**
* Reducer returning the block selection's end.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function selectionEnd( state = {}, action ) {
switch ( action.type ) {
case 'SELECTION_CHANGE':
return {
...state,
start: {
clientId: action.clientId,
attributeKey: action.attributeKey,
offset: action.startOffset,
},
end: {
clientId: action.clientId,
attributeKey: action.attributeKey,
offset: action.endOffset,
},
clientId: action.clientId,
attributeKey: action.attributeKey,
offset: action.endOffset,
};
case 'RESET_SELECTION':
return action.selectionEnd;
case 'MULTI_SELECT':
return { clientId: action.end };
}

return selection( state, action );
}

/**
* Reducer returning whether the user is multi-selecting.
*
* @param {boolean} state Current state.
* @param {Object} action Dispatched action.
*
* @return {boolean} Updated state.
*/
export function isMultiSelecting( state = false, action ) {
switch ( action.type ) {
case 'START_MULTI_SELECT':
return true;

case 'STOP_MULTI_SELECT':
return false;
}

return state;
}

/**
* Reducer returning whether selection is enabled.
*
* @param {boolean} state Current state.
* @param {Object} action Dispatched action.
*
* @return {boolean} Updated state.
*/
export function isSelectionEnabled( state = true, action ) {
switch ( action.type ) {
case 'TOGGLE_SELECTION':
return action.isSelectionEnabled;
}

return state;
}

/**
* Reducer returning the intial block selection.
*
* Currently this in only used to restore the selection after block deletion.
* This reducer should eventually be removed in favour of setting selection
* directly.
*
* @param {boolean} state Current state.
* @param {Object} action Dispatched action.
*
* @return {?number} Initial position: -1 or undefined.
*/
export function initialPosition( state, action ) {
if ( action.type === 'SELECT_BLOCK' ) {
return action.initialPosition;
} else if ( action.type === 'REMOVE_BLOCKS' ) {
return state;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This reducer is a little unconventional, in that its default behaviour isn't to return state, but rather to implicitly return undefined. Is this the best way to proceed? Seems like it could increase the likelihood of mistakes in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

How so? The reducer should store the initial position temporarily, until another action occurs. The only exception is REMOVE_BLOCKS. As mentioned above in the doc, the reducer should eventually be removed in favour of setting selection directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is more or less the same observation as in #17482 (comment)

I'll defer to your judgment, as I don't want to block this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll add a comment.


export function blocksMode( state = {}, action ) {
if ( action.type === 'TOGGLE_BLOCK_MODE' ) {
const { clientId } = action;
Expand Down Expand Up @@ -1303,7 +1320,11 @@ export default combineReducers( {
blocks,
isTyping,
isCaretWithinFormattedText,
blockSelection,
selectionStart,
selectionEnd,
isMultiSelecting,
isSelectionEnabled,
initialPosition,
blocksMode,
blockListSettings,
insertionPoint,
Expand Down
Loading