Fix undo trap in Gallery block when transforming from shortcode #42001
Conversation
This file contains hidden or 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
kevin940726
added
the
[Type] Bug
|
Size Change: +633 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
fix/gallery-undo-trap-from-raw
branch
from
f913f4e to
0b47ac7
Compare
kevin940726
force-pushed
the
fix/gallery-undo-trap-from-raw
branch
from
54b586a to
6e33868
Compare
kevin940726
force-pushed
the
fix/gallery-undo-trap-from-raw
branch
from
6e33868 to
54cff6a
Compare
kevin940726
force-pushed
the
fix/gallery-undo-trap-from-raw
branch
from
1098eac to
5337c20
Compare
kevin940726
commented
Jun 30, 2022
| showBlockToolbar = showBlockToolbar; | ||
| clickBlockOptionsMenuItem = clickBlockOptionsMenuItem.bind( this ); | ||
| clickBlockToolbarButton = clickBlockToolbarButton.bind( this ); | ||
| getEditedPostContent = getEditedPostContent.bind( this ); |
Member
Author
There was a problem hiding this comment.
This is to allow usage like the following:
await expect.poll( editor.getEditedPostContent ).toMatchSnapshot();| }, blockRepresentation ); | ||
| } | ||
|
|
||
| export type { BlockRepresentation }; |
Member
Author
There was a problem hiding this comment.
Without this, TypeScript will throw an error like Public property 'insertBlock' of exported class has or is using name 'BlockRepresentation' from external module "/Users/kai/work/gutenberg/packages/e2e-test-utils-playwright/src/editor/insert-block" but cannot be named.
Mamaduka
reviewed
Jun 30, 2022
| }, | ||
| }, | ||
| }, | ||
| __experimentalTransform( { named: { ids, columns = 3, link } } ) { |
Member
There was a problem hiding this comment.
The transformations already have transform method, so when this is stabilized we'll have name colision.
Member
Author
There was a problem hiding this comment.
I don't think the shortcode type has a transform method? Or is there something else I'm missing 😅?
Contributor
There was a problem hiding this comment.
Yep, shortcode transforms don't currently support the transform method, so this would be introducing parity with block, file and other transforms. I think it's a good idea.
I think it'd be ok to ship it as stable, btw. The only part I'm unsure about is what named: means as I haven't done a shortcode transform before. What other keys can be in that object?
But other than that the API seems to be pretty much what's expected, so I think it's ok to ship it and document it.
Contributor
There was a problem hiding this comment.
I think named: is used to indicate that it is a named shortcode attribute and not a numeric shortcode attribute.
Member
There was a problem hiding this comment.
No, you're right. I forgot that each type had its schema.
I agree with @talldan that it's ok to ship this as stable.
Member
Author
There was a problem hiding this comment.
Thanks for the feedback! I've updated the code to stabilize transform. Unfortunately, it doesn't play nice with the existing attributes field, and some of the features in attributes can't easily be done in transform (selectors).
I decided to only allow one at a time for now, but let me know if anyone has concerns or better ideas!
glendaviesnz
reviewed
Jul 1, 2022
| * | ||
| * @return {Array} An array of media info options for each gallery image. | ||
| */ | ||
| export default function useGetMedia( innerBlockImages ) { |
Contributor
There was a problem hiding this comment.
This is a nice cleanup - thanks for doing this - I think a lot of the superfluous code in this hook was a hangover from when we were making individual getMedia calls for each image before the getMediaItems endpoint was updated to allow us to do it in one call - much cleaner and simpler now, and worked as expected in my testing.
Contributor
|
This tests well for me, and is a great improvement on the existing temporary kludge we put in place to get the Gallery refactor over the line - thanks for tidying it up. Happy to give a sign off on this when the question about the naming of the transform method is sorted. |
kevin940726
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar and
ryanwelcher
as code owners
kevin940726
force-pushed
the
fix/gallery-undo-trap-from-raw
branch
from
664a6ab to
0b85220
Compare
Contributor
|
This is mostly testing well for me, except the Image captions are not copied across, but they are on trunk: Trunk: scg-before.mp4This branch: gsc-after.mp4 |
Member
Author
|
Good catch! I'll take a look. 👀 |
glendaviesnz
approved these changes
Jul 4, 2022
bph
added
the
[Block] Gallery
4 tasks
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?
Related to #8119.
Fix "undo trap" in the Gallery block when transforming from shortcode.
Why?
See the original issue for more info. When pasting shortcode like
[gallery ids="1,2,3"]to the editor, the shortcode will be transformed into a gallery block, but it's unable to undo it by using the Undo/Redo button.How?
The key here is that the
updateImages( shortCodeImages )call in theuseEffectwill schedule an update to set the attributes for its children and thus creating new undo levels automatically after the first render. TheuseEffecthere (along with otheruseEffects in the same component) is an anti-pattern and is redundant.The solution is to get rid of the
useEffectand directly inject the innercore/imageblocks instead. It requires us to update the shortcode converter to include a__experimentalTransformfunction so that we can define the transformedinnerBlocks. Similar patterns can be seen in other transformation methods like the blocks transformer. I think it makes sense to add it to the shortcode transformer too.Testing Instructions
ids.idswith yours):Screenshots or screencast
Kapture.2022-06-28.at.16.40.44.mp4
Kapture.2022-06-28.at.16.57.16.mp4