Try: removing the multi-block type check to make __experimentalConvert more useful #22577
Conversation
|
Size Change: +417 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
| ! isBlockSelectionOfSameType( blocksArray ) | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think initially, this was needed to limit the "paragraph -> list" conversion to paragraph only selections. It doesn't seem like the removal impacts this as we can't transform any multiselection to lists yet.
This is me saying, I agree with this change conceptually but how do you think we should define that a conversion is liimited to the same types or not?
There was a problem hiding this comment.
how do you think we should define that a conversion is liimited to the same types or not?
What about adding a new property like sameBlockTypes to the transform?
transforms: {
from: [
{
type: 'block',
isMultiBlock: true,
blocks: [ '*' ],
sameBlockTypes: true
__experimentalConvert( blocks ) { // ... },
},
},
}
switchToBlockType can then check the value of that property before applying the conversion:
diff --git a/packages/blocks/src/api/factory.js b/packages/blocks/src/api/factory.js
index 8775b6f78e..762a5c4d15 100644
--- a/packages/blocks/src/api/factory.js
+++ b/packages/blocks/src/api/factory.js
@@ -449,6 +449,9 @@ export function switchToBlockType( blocks, name ) {
let transformationResults;
if ( transformation.isMultiBlock ) {
+ if ( transformation.sameBlockTypes && ! isBlockSelectionOfSameType( blocksArray ) ) {
+ return null;
+ }
if ( has( transformation, '__experimentalConvert' ) ) {
transformationResults = transformation.__experimentalConvert(
blocksArrayThere was a problem hiding this comment.
Naming is always difficult but I like the idea @mmtr !
| if ( | ||
| ! isContainerGroupBlock( name ) && | ||
| isMultiBlock && | ||
| ! isBlockSelectionOfSameType( blocksArray ) |
There was a problem hiding this comment.
Pretty sure this function isBlockSelectionOfSameType becomes unused after this change (exported, but only for purpose of unit testing).
aduth
added
[Feature] Block API
|
Previously: #14908 @getdave Would you have any additional context regarding if / why the restriction here is necessary? (relevant also: #22577 (comment)) |
|
One thing that might be odd to handle is ungrouping. The Group transform has its corresponding Ungroup menu, to bring those blocks back to their standalone state. With Premium Content as an example, the user can only delete the whole Premium Content block (and all the blocks within it). Would we add an "Undo Premium Content" menu item, or allow allow reuse of the "Ungroup" item?.. Without an equivalent "ungroup"-type action, the user could copy and paste each block out of the Premium Content block, but it's kinda awkward compared to the similarly-tasked Group block. One other consideration might be to have the block-moving arrows not bounded by their containing block, so that blocks could be moved in and out that way instead (but I'm assuming that's more technically complicated). |
|
That's a nice thing to consider, @kwight. I see the Ungroup button is intentionally tidied to the Group block. Opening it to any block would allow users to ungroup any block with nested blocks causing unexpected behavior on blocks that have a parent restriction (like I guess we could disable it on blocks having that restriction, but I'm assuming users will still find the UI confusing if we stick to the "Ungroup" term since there has not been any grouping action before. Instead, I think that any block interested in pulling its inner blocks out with a user-facing button should implement its own ungroup button to be added to its block toolbar. That's possible with the current API, so in the case of the Premium Content block it should be addressed in Automattic/wp-calypso#42548 (or as a follow up). |
Just noting the "Undo" button is still available and it will revert any block transformation.
|
Oh true @kwight! The current work around would likely be adding a transform from custom block to Group transform then ungroup but that's rather awkward. We can maybe defer for future updates unless there's a need for it. Another common block case to consider for |
|
@youknowriad @aduth If we're open to the change here, is work left then:
We'd be happy to update the PR if so. |
|
I think it's fine @gwwar but do you think we can do a quick summary of the pros/cons compared to the existing approach? |
Problem:@youknowriad During block registration if we specify the following transform for a block other than "core/group" and make a selection of mixed block types say: (one paragraph, one image, and one heading) instead of (three paragraphs), we still have the option in the inserter to transform the selection to the block, however we will run into a JS error before the gutenberg/packages/blocks/src/api/factory.js Lines 413 to 416 in 281464a or TL;DR no other block other than group can transform mixed block selections. transforms: {
from: [ {
type: 'block',
isMultiBlock: true,
blocks: [ '*' ],
__experimentalConvert( blocks ) {As for the pros, @getdave do you recall what specific case we're guarding against here for the group block? We can probably guard against that in a different way. |
|
I actually cannot remember why I added that check. It was obviously a very deliberate act though as indicated by the code structure and comment. I wish past me had provided more detail on the WHY and not the WHAT! I wish I could be of more help. |
|
Thanks @gwwar that's helpful. Given the difficulty to identify the original reasons for the check, it seems like the plan here #22577 (comment) is a good one.
I know I suggested this but I did so because I thought that it was the reason the check existed here in the first place, if there's no use-case for this right now, it's fine to avoid it IMO. |
Yes I believe so! Thanks, I'll update the summary for clarity on the whys and next steps. |
gwwar
force-pushed
the
try/remove-multi-type-check
branch
from
9ca1f94
to
1fbbb28
Compare
|
This one is ready for review @youknowriad @mmtr @kwight @getdave |
|
Thanks I can tidy that. I suppose prettier doesn't run as a pre-commit?
|
|
We have a green build ✨ Thanks for the reviews @youknowriad @getdave @aduth @mmtr @kwight ! |
Problem:
During block registration if we specify the following transform for a block other than "core/group" and make a selection of mixed block types say: (one paragraph, one image, and one heading) instead of (three paragraphs), we still have the option in the inserter to transform the selection to the block, however we will run into a JS error before the
__experimentalConvertfires, due to the explicit check ingutenberg/packages/blocks/src/api/factory.js
Lines 413 to 416 in 281464a
or TL;DR no other block other than group can transform mixed block selections.
As for why the check is here, it looks like we explicitly guard for this, but @getdave and others don't recall why this is the case. That being the case, we'll try removing this limitation but keep an eye out for any Group block regressions during testing.
Example Block Use Case:
Automattic/wp-calypso#42548
Testing Instructions
npm run test-unit -- --testPathPattern packages/blocks/src/api/test/factory.jsis green, and new test cases make senseTODO