Transform: select all blocks if the result has more than one block #45015
Conversation
|
Size Change: +1.09 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
fix/block-switcher-multi-select
branch
from
f90a0db to
8b48865
Compare
| @@ -7,26 +7,38 @@ import type { Editor } from './index'; | |||
| * Clicks the default block appender. | |||
| * | |||
| * @param {Editor} this | |||
| * @param {string} name Block name. | |||
| * @param {string} name Block name or title. | |||
| */ | |||
| export async function transformBlockTo( this: Editor, name: string ) { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I should probably rewrite or remove the e2e-test-utils section in the MIGRATION guide. The utils are not meant to be one-to-one mapping and they don't necessarily have to behave the same. We try to match the names and the APIs because it's easier for the migration, but they should still follow the best practices. This is totally up for debate though as I mentioned in the comment 🙂.
packages/e2e-test-utils-playwright/src/editor/transform-block-to.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM, thanks! I actually had a conversation about e2e utils doing programmatically actions with @kevin940726 and it seems this is a case where it showed up(and I think we'll have many more).
In your case the previous util didn't select all the transformed blocks, like the block switcher behavior is now and we would have as alternatives:
- Sync the dispatched action to the util to multiselect(maintenance overhead)
- Manually select the transformed blocks and transform again(so bad IMO for so many reasons)
- Add a second util to do what a user would do and keep that one as well(also bad IMO)
This change seems to have broken some other tests, so when fixed - green CI, let's ship.
Why is this bad? I honestly think this is the best approach of all. This makes the test readable since we don't hide some user flow in the utils. I believe we should make explicit assertions of what we're trying to test against. Playwright's API makes it easy to do so, I think we should leverage that and restructure our tests accordingly. In this case, we're not testing if the edited content matches a specific output after some operations (snapshot testing). Instead, we want to test if the blocks get selected after the transformation: we should explicitly test that. The "transform" flow can be done in two steps when the block tools bar is opened. await page
.getByRole( 'region', { name: 'Block tools' } ) // Targeting the block tools
.getByRole( 'button', { name: 'List' } ) // Targeting the "List" button in the block tools
.click();
await page
.getByRole( 'group', { name: 'Transform to' } ) // Targeting the "Transform to" group
.getByRole( 'menuitem', { name: 'Paragraph' } ) // Targeting the paragraph button in the group
.click();
// ...or we can use string-based selectors:
await page.click( 'role=region[name="Block tools"i] >> role=button[name="List"i]' );
await page.click( 'role=group[name="Transform to"i] >> role=menuitem[name="Paragraph"i]' );This is readable and accessible and contains no implementation details. We can just inline it in the test to test that specific user flow. What about repeating? I don't think repeating code is bad in testing, especially when there are just two lines. If we still want to make an abstraction, we can do that in the same file with a POM-style fixture. If it's repeating in many tests then we can abstract them away into the The ideal assertion we want to make in this test is to check for the selected blocks, we can use public APIs for that. const selectedBlocksNames = await page.evaluate( () => {
return window.wp.data.select( 'core/block-editor' )
.getMultiSelectedBlocks()
.map( ( block ) => block.name );
} );
// Or something more semantic then checking block names.
expect( selectedBlocksNames ).toEqual( [ 'core/paragraph', 'core/paragraph' ] );Whether or not to repeat the transformation operation again is optional (though I think adding it is a nice touch!). We should still keep the explicit assertions there for better readability, and we can also skip snapshot testing altogether. IMO, snapshot testing is only useful if the output is directly related to the assertions. Making long and multiple snapshots is hard to review and read. With all that said, these are just my suggestions and they are up for discussion. I recommend reading the current best practices guide and raise any issues if you spot something that you disagree with. Hope that we can reach a consensus and eliminate the confusing parts :). |
| function selectForMultipleBlocks( insertedBlocks ) { | ||
| if ( insertedBlocks.length > 1 ) { | ||
| multiSelect( | ||
| insertedBlocks[ 0 ].clientId, | ||
| insertedBlocks[ insertedBlocks.length - 1 ].clientId | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -7,26 +7,38 @@ import type { Editor } from './index'; | |||
| * Clicks the default block appender. | |||
| * | |||
| * @param {Editor} this | |||
| * @param {string} name Block name. | |||
| * @param {string} name Block name or title. | |||
| */ | |||
| export async function transformBlockTo( this: Editor, name: string ) { | |||
There was a problem hiding this comment.
I should probably rewrite or remove the e2e-test-utils section in the MIGRATION guide. The utils are not meant to be one-to-one mapping and they don't necessarily have to behave the same. We try to match the names and the APIs because it's easier for the migration, but they should still follow the best practices. This is totally up for debate though as I mentioned in the comment 🙂.
| await page | ||
| .getByRole( 'button', { | ||
| name: 'List', | ||
| description: 'List: Change block type or style', | ||
| } ) | ||
| .click(); |
There was a problem hiding this comment.
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:
| 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see!
I think the description field is never a valid option in getByRole?
There was a problem hiding this comment.
Ah! I was looking at the wrong API docs. Strange that it's not an option here, it would be useful.
There was a problem hiding this comment.
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( 'button', { name: 'List' } ).click(); | ||
| await page.getByRole( 'menuitem', { name: 'Paragraph' } ).click(); | ||
|
|
||
| expect( await editor.getEditedPostContent() ).toMatchSnapshot(); |
There was a problem hiding this comment.
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.
What?
Fixes #13600. When a transform results in more than one block, select all those blocks so the result is clear and it can easily be transformed back.
Why?
How?
Testing Instructions
Screenshots or screencast