Check that a transform matches at the time of running the transform #40497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…as well as when generating list of transforms for menu
glendaviesnz
added
the
[Feature] Block API
|
Size Change: +643 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
@jorgefilipecosta, @youknowriad it was a long time ago that this transform code was added, but do you remember if there was a specific reason why it didn't check if a transform had a passing |
|
This works as advertised @glendaviesnz for cover blocks with images. One thing I noticed, and I'm currently checking to see if it's an existing issue with trunk or #40293, is that custom colors are not transferred to the group block wrapper for cover blocks with background colors and no images. Before transform <!-- wp:cover {"dimRatio":50,"customOverlayColor":"#d7a06c","isDark":false} -->
<div class="wp-block-cover is-light"><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#d7a06c"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Group</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->After transform <!-- wp:group -->
<div class="wp-block-group"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->Edit:
So far it's only happening in #40293 |
I guess that's the only reason. Is this not true, I mean it's not clear to me by reading the PR description why a transform would be isMatch "true" when showing the list but "false" when running it? |
Sorry @youknowriad, my description was not very clear. The Let me know if that doesn't clarify it enough. |
|
Oh, makes sense thank you :) |
youknowriad
reviewed
Apr 22, 2022
packages/blocks/src/api/factory.js
Outdated
| * | ||
| * @return {boolean} True if given blocks are a match for given tranform. | ||
| */ | ||
| function checkTransformIsMatch( transform, blocks, sourceBlock ) { |
There was a problem hiding this comment.
Seems like blocks or sourceBlock is the same thing right (source block or blocks)?, can't we just use a single argument for this?
There was a problem hiding this comment.
Exactly source block is just the first block in blocks const sourceBlock = first( blocks );, we can have a single parameter.
There was a problem hiding this comment.
thanks @youknowriad & @jorgefilipecosta, good point about the unnecessary argument, I have tidied this up.
…om blocks array as source block
aaronrobertshaw
approved these changes
Apr 26, 2022
There was a problem hiding this comment.
Thanks for putting this one together @glendaviesnz 👍
This tested well for me including following the testing instructions for #40293
✅ Unit tests pass
✅ Cover block with background image is transformed to Group with inner Cover block
✅ Cover with background color or gradient is transformed to Group with correct background with no inner Cover block
While testing a Cover block with a background image, it successfully transforms to Group block that wraps the current Cover block to prevent losing the image. When transforming the Group that now wraps the Cover block, back into a Cover block we end up with a Cover > Group > Cover block.
This isn't specific to this PR. I believe a simple check in the Cover block from core/group transform could clean this up. I've created a separate PR to address this: #40602.
This is completely optional but we might be able to simplify some of the conditional checks for whether the transform's isMatch is a function by moving that to the new checkTransformIsMatch function.
Example moving `isMatch` function check into `checkTransformIsMatch`
diff --git a/packages/blocks/src/api/factory.js b/packages/blocks/src/api/factory.js
index b5a39c1640..6cdf56464e 100644
--- a/packages/blocks/src/api/factory.js
+++ b/packages/blocks/src/api/factory.js
@@ -220,10 +220,7 @@ const isPossibleTransformForSource = ( transform, direction, blocks ) => {
}
// If the transform has a `isMatch` function specified, check that it returns true.
- if (
- isFunction( transform.isMatch ) &&
- ! checkTransformIsMatch( transform, blocks )
- ) {
+ if ( ! checkTransformIsMatch( transform, blocks ) ) {
return false;
}
@@ -463,13 +460,17 @@ export function getBlockTransforms( direction, blockTypeOrName ) {
* @return {boolean} True if given blocks are a match for given tranform.
*/
function checkTransformIsMatch( transform, blocks ) {
- const sourceBlock = first( blocks );
- const attributes = transform.isMultiBlock
- ? blocks.map( ( block ) => block.attributes )
- : sourceBlock.attributes;
- const block = transform.isMultiBlock ? blocks : sourceBlock;
+ if ( typeof transform.isMatch !== 'function' ) {
+ return true;
+ }
- return transform.isMatch( attributes, block );
+ if ( transform.isMultiBlock ) {
+ const attributes = blocks.map( ( block ) => block.attributes );
+ return transform.isMatch( attributes, blocks );
+ }
+
+ const sourceBlock = first( blocks );
+ return transform.isMatch( sourceBlock.attributes, sourceBlock );
}
/**
@@ -499,8 +500,7 @@ export function switchToBlockType( blocks, name ) {
( isWildcardBlockTransform( t ) ||
t.blocks.indexOf( name ) !== -1 ) &&
( ! isMultiBlock || t.isMultiBlock ) &&
- ( ! isFunction( t.isMatch ) ||
- checkTransformIsMatch( t, blocksArray ) )
+ checkTransformIsMatch( t, blocksArray )
) ||
findTransform(
transformationsFrom,
@@ -509,8 +509,7 @@ export function switchToBlockType( blocks, name ) {
( isWildcardBlockTransform( t ) ||
t.blocks.indexOf( sourceName ) !== -1 ) &&
( ! isMultiBlock || t.isMultiBlock ) &&
- ( ! isFunction( t.isMatch ) ||
- checkTransformIsMatch( t, blocksArray ) )
+ checkTransformIsMatch( t, blocksArray )
);
// Stop if there is no valid transformation.I also left one small suggestion below to fix a typo.
@aaronrobertshaw I did think about this, but decided it was slightly confusing to have |
That's fair enough. I guess I was thinking you are still checking the transform's
If the current name isn't clear enough maybe, We can of course leave it as it is but there is a lot going on in some of those other checks so I leaned towards reducing that if we could. I don't feel strongly about it. |
…ethod to avoid duplication
|
@aaronrobertshaw I have pulled the function check into a renamed |
|
@glendaviesnz this is still testing well for me. 🚀 ✅ Unit tests are still passing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Checks that a transform matches at the time of running the transform as well as when generating list of transforms for the block transform menu
Why?
If more than one transform matches in given circumstance the wrong one may get picked, eg. in #40293 a new Group
totransform was being used in error even though itsisMatchfunction returned false, and becausetotransforms are given priority it was used instead of thefromGroup transform that should be used.How?
Checks that a transforms
isMatchmethod returns true when deciding which transform should run.Testing Instructions
Check out this PR in conjunction with #40293, and make sure that a Cover block with a background image is transformed to a Cover block wrapped in a Group when the Group transform is picked, but to a Group block with the Cover block background color copied to it if Cover has no background image.
Screenshots or screencast
The following are taken from #40293 with this PR applied
Before:
transform-before2.mp4
After:
transform-after.mp4