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

Blocks: Try auto-inserting mechanism in the editor #49789

Closed
wants to merge 3 commits into from
Closed

Blocks: Try auto-inserting mechanism in the editor #49789

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
9ed33e5
Blocks: Add auto-inserting mechanism
gziolo Apr 13, 2023
458bf40
Add a test block
gziolo Apr 18, 2023
2a9506e
Add a way to reset blocks with the list of auto inserted blocks
gziolo Apr 19, 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
108 changes: 106 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 @@ -8,7 +8,7 @@ import fastDeepEqual from 'fast-deep-equal/es6';
*/
import { pipe } from '@wordpress/compose';
import { combineReducers, select } from '@wordpress/data';
import { store as blocksStore } from '@wordpress/blocks';
import { store as blocksStore, createBlock } from '@wordpress/blocks';
/**
* Internal dependencies
*/
Expand Down Expand Up @@ -597,6 +597,7 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
order: mapBlockOrder( action.blocks ),
parents: new Map( mapBlockParents( action.blocks ) ),
controlledInnerBlocks: {},
autoInsertedBlocks: action?.autoInsertedBlocks || {},
};

newState.tree = new Map( state?.tree );
Expand Down Expand Up @@ -743,6 +744,92 @@ const withResetControlledBlocks = ( reducer ) => ( state, action ) => {
return reducer( state, action );
};

/**
* Higher-order reducer that auto insert blocks.
*
* @param {Function} reducer Original reducer function.
*
* @return {Function} Enhanced reducer function.
*/
const withAutoInsertBlocks = ( reducer ) => ( state, action ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this should be in the action creator instead. I believe reducer needs to be pure functions and don't rely on things like selecting stores...

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 have no idea at this stage. I'm trying to build a prototype that works with all nested editors and properly integrates with the undo/redo mechanism 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

If we were able to modify the initial parsing to have all auto-inserted blocks in place as discussed in #49789 (comment) then we won't need this part at all. In fact, it would be as simple as extending the action creator to store the list of auto-inserted block names, so we can store them later in the database together with other changes to the edited content. This way, the next time we load the same content, we can skip the auto-inserting of blocks listed earlier because they are now controlled by the user.

const {
autoInsertedBlocks,
blocks: originalBlocks,
rootClientId = '',
type,
} = action;
if (
originalBlocks?.length &&
( 'RESET_BLOCKS' === type || 'REPLACE_INNER_BLOCKS' === type )
) {
const autoInsertBlockTypes = select( blocksStore )
.getBlockTypes()
.filter( ( { autoInsert } ) => {
return autoInsert?.after?.length > 0;
} )
.reduce( ( autoInsertAccumulator, { name, autoInsert } ) => {
return autoInsert.after.reduce(
( blockNamesAccumulator, blockName ) => {
blockNamesAccumulator[ blockName ] = {
after: [
...( blockNamesAccumulator?.[ blockName ]
?.after || [] ),
name,
],
};
return blockNamesAccumulator;
},
autoInsertAccumulator?.after || {}
);
}, {} );

const previouslyAutoInsertedBlockNames = new Set(
autoInsertedBlocks
? autoInsertedBlocks?.[ rootClientId ]
: state?.autoInsertedBlocks?.[ rootClientId ]
);
const newAutoInsertedBlockNames = new Set();
const updatedBlocks = [];
originalBlocks.forEach( ( block ) => {
updatedBlocks.push( block );
if ( block.name in autoInsertBlockTypes ) {
autoInsertBlockTypes[ block.name ].after.forEach(
( autoInsertBlockName ) => {
if (
! previouslyAutoInsertedBlockNames.has(
autoInsertBlockName
)
) {
updatedBlocks.push(
createBlock( autoInsertBlockName )
);
newAutoInsertedBlockNames.add(
autoInsertBlockName
);
}
}
);
}
} );

const newState = reducer( state, {
...action,
blocks: updatedBlocks,
} );
if ( newAutoInsertedBlockNames.size === 0 ) {
return newState;
}

return reducer( newState, {
type: 'UPDATE_AUTO_INSERTED_BLOCKS',
rootClientId,
newAutoInsertedBlockNames,
} );
}

return reducer( state, action );
};

/**
* Reducer returning the blocks state.
*
Expand All @@ -760,7 +847,8 @@ export const blocks = pipe(
withBlockReset,
withPersistentBlockChange,
withIgnoredBlockChange,
withResetControlledBlocks
withResetControlledBlocks,
withAutoInsertBlocks
)( {
// The state is using a Map instead of a plain object for performance reasons.
// You can run the "./test/performance.js" unit test to check the impact
Expand Down Expand Up @@ -1185,6 +1273,22 @@ export const blocks = pipe(
}
return state;
},

autoInsertedBlocks(
state = {},
{ type, rootClientId = '', newAutoInsertedBlockNames }
) {
if ( 'UPDATE_AUTO_INSERTED_BLOCKS' === type ) {
return {
...state,
[ rootClientId ]: [
...( state[ rootClientId ] || [] ),
...newAutoInsertedBlockNames,
],
};
}
return state;
},
} );

/**
Expand Down
79 changes: 79 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 @@ -311,6 +311,7 @@ describe( 'state', () => {
} )
),
controlledInnerBlocks: {},
autoInsertedBlocks: {},
} );
expect( state.tree.get( 'chicken' ) ).not.toBe(
existingState.tree.get( 'chicken' )
Expand Down Expand Up @@ -411,6 +412,7 @@ describe( 'state', () => {
} )
),
controlledInnerBlocks: {},
autoInsertedBlocks: {},
} );
expect( state.tree.get( 'chicken' ) ).not.toBe(
existingState.tree.get( 'chicken' )
Expand Down Expand Up @@ -576,6 +578,7 @@ describe( 'state', () => {
} )
),
controlledInnerBlocks: {},
autoInsertedBlocks: {},
} );

expect( state.tree.get( '' ).innerBlocks[ 0 ] ).toBe(
Expand Down Expand Up @@ -703,6 +706,7 @@ describe( 'state', () => {
} )
),
controlledInnerBlocks: {},
autoInsertedBlocks: {},
} );

// The block object of the parent should be updated.
Expand All @@ -724,6 +728,7 @@ describe( 'state', () => {
isIgnoredChange: false,
tree: new Map(),
controlledInnerBlocks: {},
autoInsertedBlocks: {},
} );
} );

Expand Down Expand Up @@ -2372,6 +2377,80 @@ describe( 'state', () => {
} );
} );
} );

describe( 'automatically inserted blocks', () => {
beforeAll( () => {
registerBlockType( 'core/auto-inserted-block', {
save: noop,
edit: noop,
category: 'text',
title: 'auto inserted block',
autoInsert: {
after: [ 'core/test-block' ],
},
} );
} );

afterAll( () => {
unregisterBlockType( 'core/auto-inserted-block' );
} );

it( 'should automatically insert a block', () => {
const state = blocks( undefined, {
type: 'RESET_BLOCKS',
blocks: [
{
clientId: 'chicken',
name: 'core/test-block',
attributes: {},
innerBlocks: [],
},
],
} );

expect( state.byClientId.size ).toBe( 2 );
expect( Array.from( state.byClientId.values() ) ).toEqual( [
{
clientId: 'chicken',
name: 'core/test-block',
},
expect.objectContaining( {
name: 'core/auto-inserted-block',
} ),
] );
expect( state.autoInsertedBlocks ).toEqual( {
'': [ 'core/auto-inserted-block' ],
} );
} );

it( 'should not automatically insert a previously inserted block', () => {
const state = blocks( undefined, {
type: 'RESET_BLOCKS',
blocks: [
{
clientId: 'chicken',
name: 'core/test-block',
attributes: {},
innerBlocks: [],
},
],
autoInsertedBlocks: {
'': [ 'core/auto-inserted-block' ],
},
} );

expect( state.byClientId.size ).toBe( 1 );
expect( Array.from( state.byClientId.values() ) ).toEqual( [
{
clientId: 'chicken',
name: 'core/test-block',
},
] );
expect( state.autoInsertedBlocks ).toEqual( {
'': [ 'core/auto-inserted-block' ],
} );
} );
} );
} );

describe( 'insertionPoint', () => {
Expand Down
15 changes: 15 additions & 0 deletions packages/e2e-tests/plugins/block-api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@
},
} );

registerBlockType( 'e2e-tests/auto-inserted', {
title: 'Auto Inserted',
description: 'Auto-inserted test block.',
category: 'widgets',
edit() {
return 'I got auto-inserted!';
},
save() {
return 'I got auto-inserted';
},
autoInsert: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR basically implements this as a generic block editor API. It is a decent approach, maybe the best one but I'd like us to ask the question of whether that's exactly what we want.

Basically, with this approach any blocks with this config, is going to be auto-inserted regardless of the block editor instances it's used in right? Could be a post editor, a site editor, a widgets editor...

I've heard of cases where we want to auto Insert blocks but only for specific post types for instance.

This approach can still be used if the block config is "modified" dynamically but I think it's worth asking whether we want to tie this with block registration. Can we imagine another approach? What are the pros and cons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a separate discussion on how to limit where the auto-inserting mechanism gets applied. I'm trying to explore what is possible today in the block editor for now.

The most important question for me was whether we can inject blocks without having the editor mark the changes in the UI. It looks like, we have full control over that, which sounds very promising. The other concern I have is how it all would work with undo/redo, and so far, it's going terribly, but I might be simply doing something wrong as I don't know the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I'm guessing since the list of blocks you feed the editor and the one you retrieve from it when you call "reset" initially are different, it's going to create an undo level. Not entirely sure how we can avoid that at the moment, unless the auto-insertion is part of the "initial parsing"

Copy link
Member Author

@gziolo gziolo Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not entirely sure how we can avoid that at the moment, unless the auto-insertion is part of the "initial parsing"

That would be neat to send the updated HTML from the server as it would work out of the box with the block editor. It would require changes in REST API to ensure that the post content, site templates, reusable blocks, and template parts get all tweaks. We will need exactly the same logic on the server anyway to support the requirements for site visitors who never lunch the editor. Potentially we could do most of the work in PHP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sending from the server is a good solution too but personally I was referring to the parsing done in the client. Either way should work I think. But yeah we do need the server side logic for the frontend.

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 figured out you were referring to the client side. That works, too. We need to investigate whether applying changes on the server instead makes sense. The benefit would be that such an auto-inserting block would also integrate nicely with other REST API powered apps.

after: [ 'core/quote' ],
},
} );

addFilter(
'blocks.registerBlockType',
'e2e-tests/hello-world/filter-added-after-registration',
Expand Down