Gallery block: Add a filter to automatically convert transforms to and from 3rd party blocks #33861
Conversation
glendaviesnz
added
the
[Block] Gallery
glendaviesnz
requested review from
talldan,
apeatling,
ramonjd,
andrewserong,
aaronrobertshaw and
stacimc
|
Size Change: +388 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
Hi @glendaviesnz! Just trying to test this and I think I'm missing a step.
Sorry, I was missing a step: actually converting them :) |
|
I've tested this by: 1. Disabling the image refactor experiment Jetpack tiled and slideshow galleries can convert to core gallery blocks. The nested images are the same as v1 core/gallery block. before-image-refactor.mp42. Enabling the image refactor experiment Jetpack tiled and slideshow galleries can convert to core gallery blocks. The nested images are now image blocks. image-refactor-convert.mp4No JS errors. Works well.
If I understand this correctly, we should ensure that any conversions from block If so I think it's good idea to provide this default functionality.
What problems will arise if we can't offer the reverse? |
glendaviesnz
changed the title
| } ) | ||
| ); | ||
| const ids = images.map( ( { id } ) => id ); | ||
| galleryBlock.attributes.images = images; |
There was a problem hiding this comment.
This only works because this method is called twice, so on the second time around the mutated block data is used for the actual transformation. Not sure if this is behaviour that is safe to exploit in this way or not - given this block object is only transient and disappears after the transformation maybe it is ok to mutate it like this?
There was a problem hiding this comment.
The filter based approach sounds good and pragmatic to me @glendaviesnz. I'm not sure if this is particular to my testing environment, but I'm running into an issue with transforming from Jetpack Tiled Gallery to Gallery with the v2 experiment switched on. Everything looks fine in the editor after running the transformation, but when I click save draft, and then reload the page, the image blocks are throwing validation errors:
gallery-transform-image-validation-error.mp4
From the console error, it looks like the image blocks getting created in the transform are losing their attributes somewhere along the way? (I couldn't quite work out what might be causing this as you're passing in the url and id attributes when creating the block 🤔)
I'm encountering a similar issue going the other way, from a Tiled Gallery block to a normal Gallery block. In a transformed block I just get the ids added without the inner markup. Whereas added a Tiled Gallery from scratch, there's a bunch of inner markup:
| Tiled Gallery created via a transform from Gallery | Tiled Gallery created from scratch |
|---|---|
 |
 |
The result is that after transforming from a v2 Gallery block to a Tiled Gallery block, it looks okay in the editor once I've clicked on the block, but after clicking Save draft and reloading, the block appears to have no images in it.
Are you able to reproduce either of these issues in your environment? Let me know if you need any extra info, or anything else for me to test!
Thanks, I will take a look at that, definitely an issue, not just at your end. |
There was a problem hiding this comment.
Nice, confirmed that 824de6d fixes the block validation issue for me when converting from Tiled Gallery -> Gallery block. There appears to be an existing issue in Jetpack with converting from Gallery to Tiled Gallery, unrelated to this PR.
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
…he image attribute array has content before running filter
| * @return {Block} The transformed block. | ||
| */ | ||
| function updateThirdPartyTransformToGallery( block ) { | ||
| const settings = select( blockEditorStore ).getSettings(); |
There was a problem hiding this comment.
Using the global "select", "dispatch" and "subscribe" is ideally something we should avoid. It breaks when we use several block editors in one page because this is going to get the settings for the default store only which might not be the right editor we're targeting here.
I do remember that we have a very old issue somewhere related to providing access to the block editor settings in transforms, but it's unsolved at the moment.
There was a problem hiding this comment.
Here's a related discussion #14755
There was a problem hiding this comment.
Thinking more here, it's seem the only reason we need setting here is to check whether the experimental feature is enabled or not. __unstableGalleryWithImageBlocks. Maybe this is not a block editor setting but more a global WP setting in which case a global JS variable could work window. __unstableGalleryWithImageBlocks.
That said, the current approach in this PR will work exactly like a global variable because it will only target the global store. I think we all should be aware of the limitation of select dispatch and subscribe, maybe even have a lint rule forbidding their usage unless explicitly skipped but maybe it's fine here in the end. We should able to remove this check soon.
Thanks for the extra detail about the If for some reason it turned out the that the flag had to remain for longer for some reason we could look at an alternative approach then? |
| ! toBlock.name.includes( 'core/' ) | ||
| ); | ||
|
|
||
| if ( settings.__unstableGalleryWithImageBlocks && galleryBlock ) { |
There was a problem hiding this comment.
Should the __unstableGalleryWithImageBlocks setting be used for the from transform?
There's a chance the user might add a gallery block while the experiment is active, but then deactivate it and try to transform the block.
I think even once the experiment has been deactivated, the saved block will still have inner blocks. The transform would still have to convert from those inner blocks regardless of __unstableGalleryWithImageBlocks.
There was a problem hiding this comment.
I think even once the experiment has been deactivated, the saved block will still have inner blocks. The transform would still have to convert from those inner blocks regardless of
__unstableGalleryWithImageBlocks.
True, nice catch, thanks. I have removed this check from the from transform, and everything seems to work as expected now when enabling the experiment, adding a gallery, then disabling the experiment and transforming it.
glendaviesnz
deleted the
refactor/gallery-to-nested-image-blocks-transform-filter
branch
Description
With the Gallery block refactor, any transforms coming from 3rd party blocks will fail if the gallery refactor is enabled.
I think it would be good to initially provide an automated mapping for transformations rather than just expecting plugin developers to immediately update their
to->core/galleryandfrom->core/gallerytransforms, particularly since I can't see any way for a 3rd party block to easily detect if they should be transforming to the new innerBlocks version of the block or not.To test