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

Merge inner blocks if wrappers are equal #43181

Merged
merged 8 commits into from
Oct 10, 2022
Merged

Merge inner blocks if wrappers are equal #43181

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
39d5871
Merge inner blocks if wrapper are equal
ellatrix Aug 12, 2022
91c3ced
Fix list v2
ellatrix Aug 12, 2022
e794fb3
Add e2e test
ellatrix Aug 25, 2022
2f12e72
Fix test and add forward delete
ellatrix Oct 6, 2022
6dda8da
Add forward merge for end of inner blocks
ellatrix Oct 7, 2022
b84d5fb
Fix forward merge with equal root
ellatrix Oct 7, 2022
75604a0
Fix e2e test, add comments
ellatrix Oct 7, 2022
e392ed9
Adjust block switcher e2e test
ellatrix Oct 7, 2022
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
118 changes: 111 additions & 7 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,22 +296,25 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId } ) => {
};
} );

const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
const {
updateBlockAttributes,
insertBlocks,
mergeBlocks,
replaceBlocks,
toggleSelection,
__unstableMarkLastChangeAsPersistent,
moveBlocksToPosition,
removeBlock,
selectBlock,
} = dispatch( blockEditorStore );

// Do not add new properties here, use `useDispatch` instead to avoid
// leaking new props to the public API (editor.BlockListBlock filter).
return {
setAttributes( newAttributes ) {
const { getMultiSelectedBlockClientIds } =
select( blockEditorStore );
registry.select( blockEditorStore );
const multiSelectedBlockClientIds =
getMultiSelectedBlockClientIds();
const { clientId } = ownProps;
Expand All @@ -327,34 +330,135 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
},
onInsertBlocksAfter( blocks ) {
const { clientId, rootClientId } = ownProps;
const { getBlockIndex } = select( blockEditorStore );
const { getBlockIndex } = registry.select( blockEditorStore );
const index = getBlockIndex( clientId );
insertBlocks( blocks, index + 1, rootClientId );
},
onMerge( forward ) {
const { clientId, rootClientId } = ownProps;
const { getPreviousBlockClientId, getNextBlockClientId, getBlock } =
select( blockEditorStore );
const {
getPreviousBlockClientId,
getNextBlockClientId,
getBlock,
getBlockAttributes,
getBlockName,
getBlockOrder,
} = registry.select( blockEditorStore );

// For `Delete` or forward merge, we should do the exact same thing
// as `Backspace`, but from the other block.
if ( forward ) {
if ( rootClientId ) {
const nextRootClientId =
getNextBlockClientId( rootClientId );

if ( nextRootClientId ) {
// If there is a block that follows with the same parent
// block name and the same attributes, merge the inner
// blocks.
if (
getBlockName( rootClientId ) ===
getBlockName( nextRootClientId )
) {
const rootAttributes =
getBlockAttributes( rootClientId );
const previousRootAttributes =
getBlockAttributes( nextRootClientId );

if (
Object.keys( rootAttributes ).every(
( key ) =>
rootAttributes[ key ] ===
previousRootAttributes[ key ]
)
Comment on lines +369 to +373
Copy link
Contributor

Choose a reason for hiding this comment

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

You raise an interesting question here. What led you to this check? For me, the first scenario that came to mind was merging between a UL and a OL. I first expected to just merge (i.e. skip this check), but on the other hand the way that Backspace unwraps the LI first means that the next Backspace will be very obvious (merge into the prev list, regardless of UL/OL).

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, not sure if we should keep this check or not. This is generalised for all wrapper blocks. If two quotes have an empty or same citation, they can be merged, otherwise not. If two group blocks have the same background color, they can be merged, otherwise not. But you could also make the case that we should just merge them regardless of attributes, and maybe keep the attributes of the currently selected block.

Copy link
Contributor

Choose a reason for hiding this comment

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

If two quotes have an empty or same citation, they can be merged, otherwise not. If two group blocks have the same background color, they can be merged, otherwise not.

At the risk of overpinging, @jasmussen might be someone who cares about these details like we do. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have this feeling — and correct me if I'm off beat here — that there's already a great deal of complexity around this handling. In that light, I would err on the sake of simplicity, and only add checks when we know we need them.

In this case, by having a singular merging behavior — allowing it regardless of props set — we'd also never run into the question of "why did it work this one time, but not that other"?

Not a strong opinion, but it's one that makes me lean towards starting with less.

Copy link
Member Author

Choose a reason for hiding this comment

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

For me that's fine if we're ok with the potential content loss. E.g. if we're merging two quotes with cites, the cite of the quote we're merging will be lost. Is that ok? I added these checks because I though that's not ok.

) {
registry.batch( () => {
moveBlocksToPosition(
getBlockOrder( nextRootClientId ),
nextRootClientId,
rootClientId
);
removeBlock( nextRootClientId, false );
} );
return;
}
} else {
mergeBlocks( rootClientId, nextRootClientId );
return;
}
}
}

const nextBlockClientId = getNextBlockClientId( clientId );
if ( nextBlockClientId ) {

if ( ! nextBlockClientId ) {
return;
}

// Check if it's possibile to "unwrap" the following block
// before trying to merge.
const replacement = switchToBlockType(
getBlock( nextBlockClientId ),
'*'
);

if ( replacement && replacement.length ) {
replaceBlocks( nextBlockClientId, replacement );
} else {
mergeBlocks( clientId, nextBlockClientId );
}
} else {
const previousBlockClientId =
getPreviousBlockClientId( clientId );

if ( previousBlockClientId ) {
mergeBlocks( previousBlockClientId, clientId );
} else if ( rootClientId ) {
const previousRootClientId =
getPreviousBlockClientId( rootClientId );

// If there is a preceding block with the same parent block
// name and the same attributes, merge the inner blocks.
if (
previousRootClientId &&
getBlockName( rootClientId ) ===
getBlockName( previousRootClientId )
) {
const rootAttributes =
getBlockAttributes( rootClientId );
const previousRootAttributes =
getBlockAttributes( previousRootClientId );

if (
Object.keys( rootAttributes ).every(
( key ) =>
rootAttributes[ key ] ===
previousRootAttributes[ key ]
)
) {
registry.batch( () => {
moveBlocksToPosition(
getBlockOrder( rootClientId ),
rootClientId,
previousRootClientId
);
removeBlock( rootClientId, false );
} );
return;
}
}

// Attempt to "unwrap" the block contents when there's no
// preceding block to merge with.
const replacement = switchToBlockType(
getBlock( rootClientId ),
'*'
);
if ( replacement && replacement.length ) {
replaceBlocks( rootClientId, replacement, 0 );
registry.batch( () => {
replaceBlocks( rootClientId, replacement );
selectBlock( replacement[ 0 ].clientId, 0 );
} );
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/list-item/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export default function ListItemEdit( {
setAttributes,
onReplace,
clientId,
mergeBlocks,
} ) {
const { placeholder, content } = attributes;
const blockProps = useBlockProps( { ref: useCopy( clientId ) } );
Expand All @@ -70,7 +71,7 @@ export default function ListItemEdit( {
const useEnterRef = useEnter( { content, clientId } );
const useSpaceRef = useSpace( clientId );
const onSplit = useSplit( clientId );
const onMerge = useMerge( clientId );
const onMerge = useMerge( clientId, mergeBlocks );
return (
<>
<li { ...innerBlocksProps }>
Expand Down
27 changes: 4 additions & 23 deletions packages/block-library/src/list-item/hooks/use-merge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { useRegistry, useDispatch, useSelect } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';
import { getDefaultBlockName, switchToBlockType } from '@wordpress/blocks';

/**
* Internal dependencies
Expand All @@ -12,17 +11,16 @@ import useOutdentListItem from './use-outdent-list-item';

import { name as listItemName } from '../block.json';

export default function useMerge( clientId ) {
export default function useMerge( clientId, onMerge ) {
const registry = useRegistry();
const {
getPreviousBlockClientId,
getNextBlockClientId,
getBlockOrder,
getBlockRootClientId,
getBlockName,
getBlock,
} = useSelect( blockEditorStore );
const { mergeBlocks, moveBlocksToPosition, replaceBlock, selectBlock } =
const { mergeBlocks, moveBlocksToPosition } =
useDispatch( blockEditorStore );
const [ , outdentListItem ] = useOutdentListItem( clientId );

Expand Down Expand Up @@ -79,29 +77,12 @@ export default function useMerge( clientId ) {
return getBlockOrder( order[ 0 ] )[ 0 ];
}

function switchToDefaultBlockType( forward ) {
const rootClientId = getBlockRootClientId( clientId );
const replacement = switchToBlockType(
getBlock( rootClientId ),
getDefaultBlockName()
);
const indexToSelect = forward ? replacement.length - 1 : 0;
const initialPosition = forward ? -1 : 0;
registry.batch( () => {
replaceBlock( rootClientId, replacement );
selectBlock(
replacement[ indexToSelect ].clientId,
initialPosition
);
} );
}

return ( forward ) => {
if ( forward ) {
const nextBlockClientId = getNextId( clientId );

if ( ! nextBlockClientId ) {
switchToDefaultBlockType( forward );
onMerge( forward );
return;
}

Expand Down Expand Up @@ -134,7 +115,7 @@ export default function useMerge( clientId ) {
mergeBlocks( trailingId, clientId );
} );
} else {
switchToDefaultBlockType( forward );
onMerge( forward );
}
}
};
Expand Down
11 changes: 11 additions & 0 deletions packages/block-library/src/list/transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,17 @@ const transforms = {
);
},
} ) ),
{
type: 'block',
blocks: [ '*' ],
transform: ( _attributes, childBlocks ) => {
return getListContentFlat( childBlocks ).map( ( content ) =>
createBlock( 'core/paragraph', {
content,
} )
);
},
},
],
};

Expand Down
30 changes: 27 additions & 3 deletions packages/e2e-tests/specs/editor/various/splitting-merging.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,20 @@ describe( 'splitting and merging blocks', () => {
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'item 2' );
await pressKeyTimes( 'ArrowUp', 3 );
await page.keyboard.press( 'Delete' );
expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:paragraph -->
<p>hi</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>item 1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>item 2</p>
<!-- /wp:paragraph -->"
` );
await page.keyboard.press( 'Delete' );
// Carret should be in the first block and at the proper position.
await page.keyboard.type( '-' );
Expand All @@ -259,15 +273,25 @@ describe( 'splitting and merging blocks', () => {
await page.keyboard.press( 'ArrowUp' );
await pressKeyTimes( 'ArrowLeft', 6 );
await page.keyboard.press( 'Backspace' );
// Carret should be in the first block and at the proper position.
await page.keyboard.type( '-' );
expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:paragraph -->
<p>hi</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>-item 1</p>
<p>item 1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>item 2</p>
<!-- /wp:paragraph -->"
` );
await page.keyboard.press( 'Backspace' );
// Carret should be in the first block and at the proper position.
await page.keyboard.type( '-' );
expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:paragraph -->
<p>hi-item 1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
Expand Down
46 changes: 46 additions & 0 deletions test/e2e/specs/editor/blocks/list.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,4 +1150,50 @@ test.describe( 'List', () => {
// Verify no WSOD and content is proper.
expect( await editor.getEditedPostContent() ).toMatchSnapshot();
} );

test( 'should merge two list with same attributes', async ( {
editor,
page,
} ) => {
await page.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '* a' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'b' );
await page.keyboard.press( 'Enter' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( '* c' );

await expect.poll( editor.getEditedPostContent ).toBe( `<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>a</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>b</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>c</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->` );

await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'Backspace' );

await expect.poll( editor.getEditedPostContent ).toBe( `<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>a</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>b</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>c</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->` );
} );
} );