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

Add mechanism to avoid forced child selection on blocks with templates. #10696

Merged
merged 1 commit into from
Nov 7, 2018
Merged

Add mechanism to avoid forced child selection on blocks with templates. #10696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions docs/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,8 +1532,8 @@ inserted, optionally at a specific index respective a root block list.

* block: Block object to insert.
* index: Index at which block should be inserted.
* rootClientId: Optional root client ID of block list on which
to insert.
* rootClientId: Optional root client ID of block list on which to insert.
* updateSelection: If true block selection will be updated. If false, block selection will not change. Defaults to true.

### insertBlocks

Expand All @@ -1544,8 +1544,8 @@ be inserted, optionally at a specific index respective a root block list.

* blocks: Block objects to insert.
* index: Index at which block should be inserted.
* rootClientId: Optional root client ID of block list on
which to insert.
* rootClientId: Optional root cliente ID of block list on which to insert.
* updateSelection: If true block selection will be updated. If false, block selection will not change. Defaults to true.

### showInsertionPoint

Expand Down
7 changes: 7 additions & 0 deletions packages/editor/src/components/inner-blocks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ const TEMPLATE = [ [ 'core/columns', {}, [

The previous example creates an InnerBlocks area containing two columns one with an image and the other with a paragraph.

### `templateInsertUpdatesSelection`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is really needed or if it should always be "false"?

Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to be conditional. If you're building a nested block with a flow optimised for writing you would want the selection to be updated for keyboard use.

Making it always false would also be a change in the current behaviour, which updates the selection automatically.

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 agree for some blocks it may make sense to use a different setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be clearer if it's named: updateSelectionOnTemplateInsert ?

* **Type:** `Boolean`
* **Default:** `true`

If true when child blocks in the template are inserted the selection is updated.
If false the selection should not be updated when child blocks specified in the template are inserted.

### `templateLock`
* **Type:** `String|Boolean`

Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,15 @@ InnerBlocks = compose( [
insertBlocks,
updateBlockListSettings,
} = dispatch( 'core/editor' );
const { block, clientId } = ownProps;
const { block, clientId, templateInsertUpdatesSelection = true } = ownProps;

return {
replaceInnerBlocks( blocks ) {
const clientIds = map( block.innerBlocks, 'clientId' );
if ( clientIds.length ) {
replaceBlocks( clientIds, blocks );
} else {
insertBlocks( blocks, undefined, clientId );
insertBlocks( blocks, undefined, clientId, templateInsertUpdatesSelection );
}
},
updateNestedSettings( settings ) {
Expand Down
23 changes: 12 additions & 11 deletions packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,35 +295,36 @@ export function moveBlockToPosition( clientId, fromRootClientId, toRootClientId,
* Returns an action object used in signalling that a single block should be
* inserted, optionally at a specific index respective a root block list.
*
* @param {Object} block Block object to insert.
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root client ID of block list on which
* to insert.
* @param {Object} block Block object to insert.
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root client ID of block list on which to insert.
* @param {?boolean} updateSelection If true block selection will be updated. If false, block selection will not change. Defaults to true.
*
* @return {Object} Action object.
*/
export function insertBlock( block, index, rootClientId ) {
return insertBlocks( [ block ], index, rootClientId );
export function insertBlock( block, index, rootClientId, updateSelection = true ) {
return insertBlocks( [ block ], index, rootClientId, updateSelection );
}

/**
* Returns an action object used in signalling that an array of blocks should
* be inserted, optionally at a specific index respective a root block list.
*
* @param {Object[]} blocks Block objects to insert.
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root client ID of block list on
* which to insert.
* @param {Object[]} blocks Block objects to insert.
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root cliente ID of block list on which to insert.
* @param {?boolean} updateSelection If true block selection will be updated. If false, block selection will not change. Defaults to true.
*
* @return {Object} Action object.
*/
export function insertBlocks( blocks, index, rootClientId ) {
export function insertBlocks( blocks, index, rootClientId, updateSelection = true ) {
return {
type: 'INSERT_BLOCKS',
blocks: castArray( blocks ),
index,
rootClientId,
time: Date.now(),
updateSelection,
};
}

Expand Down
20 changes: 12 additions & 8 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,14 +652,18 @@ export function blockSelection( state = {
end: action.clientId,
initialPosition: action.initialPosition,
};
case 'INSERT_BLOCKS':
return {
...state,
start: action.blocks[ 0 ].clientId,
end: action.blocks[ 0 ].clientId,
initialPosition: null,
isMultiSelecting: false,
};
case 'INSERT_BLOCKS': {
if ( action.updateSelection ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this is not what's causing the innerBlocks to be selected, it's the focusTabbable function inside block.js. This on ensures the inserted block (parent) is selected.

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

But the children are inserted after the parent, so without this changes, the selection gets updated when the child blocks are inserted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh makes sense. Seems like a decent approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @youknowriad thank you for looking into this PR. Do you think it is ready to merge?

return {
...state,
start: action.blocks[ 0 ].clientId,
end: action.blocks[ 0 ].clientId,
initialPosition: null,
isMultiSelecting: false,
};
}
return state;
}
case 'REMOVE_BLOCKS':
if ( ! action.clientIds || ! action.clientIds.length || action.clientIds.indexOf( state.start ) === -1 ) {
return state;
Expand Down
2 changes: 2 additions & 0 deletions packages/editor/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ describe( 'actions', () => {
index,
rootClientId: 'testclientid',
time: expect.any( Number ),
updateSelection: true,
} );
} );
} );
Expand All @@ -204,6 +205,7 @@ describe( 'actions', () => {
index,
rootClientId: 'testclientid',
time: expect.any( Number ),
updateSelection: true,
} );
} );
} );
Expand Down
19 changes: 19 additions & 0 deletions packages/editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1499,6 +1499,7 @@ describe( 'state', () => {
clientId: 'ribs',
name: 'core/freeform',
} ],
updateSelection: true,
} );

expect( state3 ).toEqual( {
Expand All @@ -1509,6 +1510,24 @@ describe( 'state', () => {
} );
} );

it( 'should not select inserted block if updateSelection flag is false', () => {
const original = deepFreeze( { start: 'a', end: 'b' } );

const state3 = blockSelection( original, {
type: 'INSERT_BLOCKS',
blocks: [ {
clientId: 'ribs',
name: 'core/freeform',
} ],
updateSelection: false,
} );

expect( state3 ).toEqual( {
start: 'a',
end: 'b',
} );
} );

it( 'should not update the state if the block moved is already selected', () => {
const original = deepFreeze( { start: 'ribs', end: 'ribs' } );
const state = blockSelection( original, {
Expand Down