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

Update last inserted block state to track multiple blocks #46885

Merged
merged 11 commits into from
Jan 5, 2023
Merged

Update last inserted block state to track multiple blocks #46885

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
afaf6a2
Include replace in tracking last “inserted” block
getdave Jan 3, 2023
d1aeb57
Update reducer tests
getdave Jan 4, 2023
7aba184
Make selector experimental
getdave Jan 4, 2023
61c7137
Refactor state to return array of inserted blocks
getdave Jan 4, 2023
78e2d45
rename selector
Jan 4, 2023
8ad7090
Deprecates existing function
getdave Jan 5, 2023
a528159
Adds selector to return multiple blocks
getdave Jan 5, 2023
b5de919
Add tests for new selector
getdave Jan 5, 2023
c74eaf1
Fix usage to utilise new selector
getdave Jan 5, 2023
e14860f
fix selector name
Jan 5, 2023
7fe3070
Remove selector
getdave Jan 5, 2023
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
12 changes: 0 additions & 12 deletions docs/reference-guides/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,18 +556,6 @@ _Properties_
- _isDisabled_ `boolean`: Whether or not the user should be prevented from inserting this item.
- _frecency_ `number`: Heuristic that combines frequency and recency.

### getLastInsertedBlockClientId

Gets the client id of the last inserted block.

_Parameters_

- _state_ `Object`: Global application state.

_Returns_

- `string|undefined`: Client Id of the last inserted block.

### getLastMultiSelectedBlockClientId

Returns the client ID of the last block in the multi-selection set, or null
Expand Down
9 changes: 7 additions & 2 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 @@ -1829,14 +1829,19 @@ export function highlightedBlock( state, action ) {
export function lastBlockInserted( state = {}, action ) {
switch ( action.type ) {
case 'INSERT_BLOCKS':
case 'REPLACE_BLOCKS':
case 'REPLACE_INNER_BLOCKS':
if ( ! action.blocks.length ) {
return state;
}

const clientId = action.blocks[ 0 ].clientId;
const clientIds = action.blocks.map( ( block ) => {
return block.clientId;
} );

const source = action.meta?.source;

return { clientId, source };
return { clientIds, source };
case 'RESET_BLOCKS':
return {};
}
Expand Down
9 changes: 6 additions & 3 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2647,7 +2647,7 @@ export const __experimentalGetActiveBlockIdByBlockNames = createSelector(
export function wasBlockJustInserted( state, clientId, source ) {
const { lastBlockInserted } = state;
return (
lastBlockInserted.clientId === clientId &&
lastBlockInserted.clientIds?.includes( clientId ) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to preserve the intent of the existing selector as if the block has just been inserted then it will stil be a valid result.

lastBlockInserted.source === source
);
}
Expand All @@ -2658,8 +2658,11 @@ export function wasBlockJustInserted( state, clientId, source ) {
* @param {Object} state Global application state.
* @return {string|undefined} Client Id of the last inserted block.
*/
export function getLastInsertedBlockClientId( state ) {
return state?.lastBlockInserted?.clientId;
export function __experimentalGetLastInsertedBlockClientId( state ) {
return (
state?.lastBlockInserted?.clientIds?.length &&
state?.lastBlockInserted?.clientIds[ 0 ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could update this selector to return all the clientIds and allow the user to deal with which one they want to count as the "last" or "latest".

So if there is a single block you'd then consume it thus

const lastInsertedBlockClientId = __experimentalGetLastInsertedBlockClientIds()[0]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should return the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, picking the first one is very arbitrary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent. We have a consensus. So I assume we:

  • rename the selector to __experimentalGetLastInsertedBlockClientIds (plural)
  • deprecate the original selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to follow the guidance and advice in the docs

https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/backward-compatibility.md#how-to-preserve-backward-compatibility-for-a-javascript-function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I have now:

  • deprecated the original selector (this is why it should have been experimental 🤦 )
  • add a new selector which returns an array

I still don't know if the new selector should be experimental. @draganescu felt there was no need as it was in use by the offcanvas. But that itself is an experiment so ... 🤷‍♂️ Opinions appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote we keep it experimental for now.

);
}

/**
Expand Down
48 changes: 45 additions & 3 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 @@ -3255,7 +3255,7 @@ describe( 'state', () => {
} );

describe( 'lastBlockInserted', () => {
it( 'should return client id if last block inserted is called with action INSERT_BLOCKS', () => {
it( 'should contain client id if last block inserted is called with action INSERT_BLOCKS', () => {
const expectedClientId = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';

const action = {
Expand All @@ -3272,7 +3272,7 @@ describe( 'state', () => {

const state = lastBlockInserted( {}, action );

expect( state.clientId ).toBe( expectedClientId );
expect( state.clientIds ).toContain( expectedClientId );
} );

it( 'should return inserter_menu source if last block inserted is called with action INSERT_BLOCKS', () => {
Expand All @@ -3297,7 +3297,7 @@ describe( 'state', () => {

it( 'should return state if last block inserted is called with action INSERT_BLOCKS and block list is empty', () => {
const expectedState = {
clientId: '9db792c6-a25a-495d-adbd-97d56a4c4189',
clientIds: [ '9db792c6-a25a-495d-adbd-97d56a4c4189' ],
};

const action = {
Expand All @@ -3313,6 +3313,48 @@ describe( 'state', () => {
expect( state ).toEqual( expectedState );
} );

it( 'should return client ids of blocks when called with REPLACE_BLOCKS', () => {
const clientIdOne = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';
const clientIdTwo = '9db792c6-a25a-495d-adbd-97d56a4c4189';

const action = {
blocks: [
{
clientId: clientIdOne,
},
{
clientId: clientIdTwo,
},
],
type: 'REPLACE_BLOCKS',
};

const state = lastBlockInserted( {}, action );

expect( state.clientIds ).toEqual( [ clientIdOne, clientIdTwo ] );
} );

it( 'should return client ids of all blocks when inner blocks are replaced with REPLACE_INNER_BLOCKS', () => {
const clientIdOne = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';
const clientIdTwo = '9db792c6-a25a-495d-adbd-97d56a4c4189';

const action = {
blocks: [
{
clientId: clientIdOne,
},
{
clientId: clientIdTwo,
},
],
type: 'REPLACE_INNER_BLOCKS',
};

const state = lastBlockInserted( {}, action );

expect( state.clientIds ).toEqual( [ clientIdOne, clientIdTwo ] );
} );

it( 'should return empty state if last block inserted is called with action RESET_BLOCKS', () => {
const expectedState = {};

Expand Down
10 changes: 5 additions & 5 deletions packages/block-editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ const {
__experimentalGetPatternTransformItems,
wasBlockJustInserted,
__experimentalGetGlobalBlocksByName,
getLastInsertedBlockClientId,
__experimentalGetLastInsertedBlockClientId: getLastInsertedBlockClientId,
} = selectors;

describe( 'selectors', () => {
Expand Down Expand Up @@ -4457,7 +4457,7 @@ describe( 'selectors', () => {

const state = {
lastBlockInserted: {
clientId: expectedClientId,
clientIds: [ expectedClientId ],
source,
},
};
Expand All @@ -4474,7 +4474,7 @@ describe( 'selectors', () => {

const state = {
lastBlockInserted: {
clientId: unexpectedClientId,
clientIds: [ unexpectedClientId ],
source,
},
};
Expand All @@ -4490,7 +4490,7 @@ describe( 'selectors', () => {

const state = {
lastBlockInserted: {
clientId,
clientIds: [ clientId ],
},
};

Expand Down Expand Up @@ -4679,7 +4679,7 @@ describe( 'getLastInsertedBlockClientId', () => {
it( 'should return clientId if blocks have been inserted', () => {
const state = {
lastBlockInserted: {
clientId: '123456',
clientIds: [ '123456' ],
},
};

Expand Down