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

Jump to bottom

Gallery block: Add a filter to automatically convert transforms to and from 3rd party blocks #33861

Merged
merged 12 commits into from Aug 12, 2021
Merged

Gallery block: Add a filter to automatically convert transforms to and from 3rd party blocks #33861

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
51c543b
Add a filter to automatically convert transforms from 3rd party block…
Aug 3, 2021
a5a100b
Changes from code review
Aug 4, 2021
64739e2
Add a transform to filter for 3rd party blocks
Aug 4, 2021
aeb6d7b
Keep innerBlocks set so 3rd party blocks can migrate to it
Aug 5, 2021
9418d37
Fix doc block spacing
Aug 5, 2021
91a04e2
Include alt attribute
Aug 5, 2021
824de6d
make sure the image id is an int
Aug 6, 2021
5cb002f
fix typo
glendaviesnz Aug 6, 2021
6635d42
Fix copy and paste error
glendaviesnz Aug 6, 2021
f1083a8
Fix typo
glendaviesnz Aug 6, 2021
17a4030
Check that id is truthy before trying to parse, and also check that t…
Aug 6, 2021
e26be69
Remove check for `__unstableGalleryWithImageBlocks ` in from gallery …
Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 93 additions & 0 deletions packages/block-library/src/gallery/transforms.js
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import { createBlock } from '@wordpress/blocks';
import { createBlobURL } from '@wordpress/blob';
import { select } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';
import { addFilter } from '@wordpress/hooks';

/**
* Internal dependencies
Expand All @@ -33,6 +34,98 @@ const parseShortcodeIds = ( ids ) => {
return ids.split( ',' ).map( ( id ) => parseInt( id, 10 ) );
};

/**
* Third party block plugins don't have an easy way to detect if the
* innerBlocks version of the Gallery is running when they run a
* 3rdPartyBlock -> GallaryBlock transform so this tranform filter
glendaviesnz marked this conversation as resolved.
Show resolved Hide resolved
* will handle this. Once the innerBlocks version is the default
* in a core release, this could be deprecated and removed after
* plugin authors have been given time to update transforms.
*
* @typedef {Object} Attributes
* @typedef {Object} Block
* @property {Attributes} attributes The attributes of the block.
* @param {Block} block The transformed block.
* @return {Block} The transformed block.
*/
function updateThirdPartyTransformToGallery( block ) {
const settings = select( blockEditorStore ).getSettings();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a related discussion #14755

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (
settings.__unstableGalleryWithImageBlocks &&
block.name === 'core/gallery' &&
block.attributes?.images
) {
const innerBlocks = block.attributes.images.map(
( { url, id, alt } ) => {
return createBlock( 'core/image', {
url,
id: parseInt( id, 10 ),
alt,
sizeSlug: block.attributes.sizeSlug,
linkDestination: block.attributes.linkDestination,
} );
}
);

delete block.attributes.ids;
delete block.attributes.images;
block.innerBlocks = innerBlocks;
}

return block;
}
addFilter(
'blocks.switchToBlockType.transformedBlock',
'core/gallery/update-third-party-transform-to',
updateThirdPartyTransformToGallery
);

/**
* Third party block plugins don't have an easy way to detect if the
* innerBlocks version of the Gallery is running when they run a
* 3rdPartyBlock -> GallaryBlock transform so this tranform filter
glendaviesnz marked this conversation as resolved.
Show resolved Hide resolved
* will handle this. Once the innerBlocks version is the default
* in a core release, this could be deprecated and removed after
* plugin authors have been given time to update transforms.
*
* @typedef {Object} Attributes
* @typedef {Object} Block
* @property {Attributes} attributes The attributes of the block.
* @param {Block} toBlock The block yo transform to.
* @param {Block[]} fromBlocks The block yo transform to.
glendaviesnz marked this conversation as resolved.
Show resolved Hide resolved
* @return {Block} The transformed block.
*/
function updateThirdPartyTransformFromGallery( toBlock, fromBlocks ) {
const settings = select( blockEditorStore ).getSettings();
const galleryBlock = fromBlocks.find(
( transformedBlock ) =>
transformedBlock.name === 'core/gallery' &&
transformedBlock.innerBlocks.length > 0 &&
! transformedBlock.attributes.images?.length > 0 &&
! toBlock.name.includes( 'core/' )
);

if ( settings.__unstableGalleryWithImageBlocks && galleryBlock ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const images = galleryBlock.innerBlocks.map(
( { attributes: { url, id, alt } } ) => ( {
url,
id: parseInt( id, 10 ),
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
alt,
} )
);
const ids = images.map( ( { id } ) => id );
galleryBlock.attributes.images = images;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

galleryBlock.attributes.ids = ids;
}

return toBlock;
}
addFilter(
'blocks.switchToBlockType.transformedBlock',
'core/gallery/update-third-party-transform-from',
updateThirdPartyTransformFromGallery
);

const transforms = {
from: [
{
Expand Down