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

Transform: select all blocks if the result has more than one block #45015

Merged
merged 6 commits into from
Nov 3, 2022
Merged

Transform: select all blocks if the result has more than one block #45015

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e5705c
Transform: select all blocks if the result has more than one block
ellatrix Oct 17, 2022
8b48865
Add e2e test
ellatrix Nov 2, 2022
28b1223
Pass variable to evaluate
ellatrix Nov 2, 2022
093b38a
Revert transformToBlock changes
ellatrix Nov 3, 2022
55f03fa
Simplify
ellatrix Nov 3, 2022
ceadd20
Remove invalid description option
ellatrix Nov 3, 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
23 changes: 19 additions & 4 deletions packages/block-editor/src/components/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import PatternTransformationsMenu from './pattern-transformations-menu';
import useBlockDisplayTitle from '../block-title/use-block-display-title';

export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
const { replaceBlocks } = useDispatch( blockEditorStore );
const { replaceBlocks, multiSelect } = useDispatch( blockEditorStore );
const blockInformation = useBlockDisplayInformation( blocks[ 0 ].clientId );
const {
possibleBlockTransformations,
Expand Down Expand Up @@ -89,12 +89,27 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
const isReusable = blocks.length === 1 && isReusableBlock( blocks[ 0 ] );
const isTemplate = blocks.length === 1 && isTemplatePart( blocks[ 0 ] );

function selectForMultipleBlocks( insertedBlocks ) {
if ( insertedBlocks.length > 1 ) {
multiSelect(
insertedBlocks[ 0 ].clientId,
insertedBlocks[ insertedBlocks.length - 1 ].clientId
);
}
}
Comment on lines +92 to +99
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be good to be added to the replaceBlocks thunk so that everywhere else will get the consistent behavior? The insertBlocks action has a similar updateSelection optional options to this, so it might make sense to do something similar to replaceBlocks as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I would love to do as well, but we can't change the behaviour now, unless we introduce yet another option to select all blocks. I think we shouldn't do that just yet until we find more use cases.


// Simple block tranformation based on the `Block Transforms` API.
const onBlockTransform = ( name ) =>
replaceBlocks( clientIds, switchToBlockType( blocks, name ) );
function onBlockTransform( name ) {
const newBlocks = switchToBlockType( blocks, name );
replaceBlocks( clientIds, newBlocks );
selectForMultipleBlocks( newBlocks );
}

// Pattern transformation through the `Patterns` API.
const onPatternTransform = ( transformedBlocks ) =>
function onPatternTransform( transformedBlocks ) {
replaceBlocks( clientIds, transformedBlocks );
selectForMultipleBlocks( transformedBlocks );
}

/**
* The `isTemplate` check is a stopgap solution here.
Expand Down
7 changes: 7 additions & 0 deletions .../e2e/specs/editor/blocks/__snapshots__/List-selects-all-transformed-output-1-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>2</p>
<!-- /wp:paragraph -->
9 changes: 9 additions & 0 deletions .../e2e/specs/editor/blocks/__snapshots__/List-selects-all-transformed-output-2-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>2</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->
9 changes: 9 additions & 0 deletions .../e2e/specs/editor/blocks/__snapshots__/List-selects-all-transformed-output-3-chromium.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>2</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->
34 changes: 34 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 @@ -1231,4 +1231,38 @@ test.describe( 'List', () => {

expect( await editor.getEditedPostContent() ).toMatchSnapshot();
} );

test( 'selects all transformed output', async ( { editor, page } ) => {
await editor.insertBlock( {
name: 'core/list',
innerBlocks: [
{ name: 'core/list-item', attributes: { content: '1' } },
{ name: 'core/list-item', attributes: { content: '2' } },
],
} );

await editor.selectBlocks(
page.locator( 'role=document[name="Block: List"i]' )
);

await page
.getByRole( 'button', {
name: 'List',
description: 'List: Change block type or style',
} )
.click();
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 the description field is only needed because we're not limiting the results. Selectors are strict by default, which means if a selector matches multiple elements then it will fail. We can chain selectors to filter out those that are not what we're targeting.

In this case, I think we can do this:

Suggested change
await page
.getByRole( 'button', {
name: 'List',
description: 'List: Change block type or style',
} )
.click();
await page
.getByRole( 'region', { name: 'Block tools' } )
.getByRole( 'button', { name: 'List' } )
.click();

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that the description was needed, it's that I'd like to use it over more specificity. I would have liked to simply use page.getByRole( 'button', { description: 'List: Change block type or style' } ), but it did not work ("Error: strict mode violation: "role=button" resolved to 20 elements"). Do you know why? I though the description should limit the results to 1.

Copy link
Member

Choose a reason for hiding this comment

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

I see!

I think the description field is never a valid option in getByRole?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! I was looking at the wrong API docs. Strange that it's not an option here, it would be useful.

Copy link
Member

@kevin940726 kevin940726 Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I still think we should chain selectors here though. Selecting getByRole('button', { name: 'List' }) is not immediately readable for which element exactly is being selected. The test will break if there's a random button with the same name added to the page. To increase the specificity level, we should make it clear that the button is from the Block tools region as suggested above. WDYT?

await page.getByRole( 'menuitem', { name: 'Paragraph' } ).click();

expect( await editor.getEditedPostContent() ).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

As per #45015 (comment), I don't think snapshot testing makes much sense here. The assertions here are not readable and feel disconnected from the title of the test case. Instead, we should prioritize explicit assertions and make them target the selected blocks. The same goes for the last snapshot assertion in this test case.


await page
.getByRole( 'button', {
name: 'Paragraph',
description: 'Change type of 2 blocks',
} )
.click();
await page.getByRole( 'menuitem', { name: 'List' } ).click();

expect( await editor.getEditedPostContent() ).toMatchSnapshot();
} );
} );