-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add possibility to manage innerBlocks while migrating deprecated blocks. #5932
Add possibility to manage innerBlocks while migrating deprecated blocks. #5932
Conversation
8edc1bb
to
aa7fc43
Compare
fa54943
to
59efeb4
Compare
blocks/api/parser.js
Outdated
@@ -181,12 +181,14 @@ export function getAttributesFromDeprecatedVersion( blockType, innerHTML, attrib | |||
try { | |||
// Handle migration of older attributes into current version if necessary. | |||
const deprecatedBlockAttributes = getBlockAttributes( deprecatedBlockType, innerHTML, attributes ); | |||
const migratedBlockAttributes = deprecatedBlockType.migrate ? deprecatedBlockType.migrate( deprecatedBlockAttributes ) : deprecatedBlockAttributes; | |||
const migratedBlockAttributesAndInnerBlocks = deprecatedBlockType.migrate ? | |||
deprecatedBlockType.migrate( { attributes: deprecatedBlockAttributes, innerBlocks } ) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny that to avoid breaking changes, we're introducing another breaking change here :) (especially the return value)
Options here:
- We're ok with this, assuming people didn't use
migrate
yet. - We're not ok and we try to support the old API. Here's how I would achieve it:
- Instead of passing an object, pass the
innerBlocks
as a second argument. - If you want to update both attributes and innerBlocks, return an array with two elements, if you return just an object assume we update only the attributes.
- Instead of passing an object, pass the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still forming an opinion, but leaning toward number 2. In practice, I doubt that migrate
has been used a lot in the wild up till now, but that's entirely speculative (thus epistemically irresponsible? 😄), and I think it's bad karma to be changing a migration API at this stage.
59efeb4
to
991e56c
Compare
aa7fc43
to
291d97c
Compare
5789d23
to
da419d3
Compare
7059339
to
21545b9
Compare
da419d3
to
df69227
Compare
21545b9
to
38bcf8d
Compare
This PR was updated to be back compatible I followed the suggestion and now we receive two arguments and return an array with attributes and inner blocks or an object with just the attributes. |
blocks/api/parser.js
Outdated
const migratedBlockAttributesAndInnerBlocks = deprecatedBlockType.migrate && | ||
deprecatedBlockType.migrate( deprecatedBlockAttributes, innerBlocks ); | ||
|
||
if ( isArray( migratedBlockAttributesAndInnerBlocks ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Can we simplify the logic here or is it too clever?
const [ migratedAttributes, migratedInnerBlocks = innerBlocks ] = castArray( migratedBlockAttributesAndInnerBlocks );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this version castArray simplifies the logic, I updated to use it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
df69227
to
e541d8c
Compare
5d5d9ad
to
fdc77b1
Compare
Thank you for the reviews @youknowriad and @mcsf! |
fdc77b1
to
8fb5d8e
Compare
8fb5d8e
to
8e46963
Compare
…ks. Added migration logic to migrate existing cover images into the new nested version. Before the migrate function received the existing attributes and returned the new attributes now, migrate receives an object with attributes and innerBlocks and returns another object with the migrated attributes & innerBlocks.
8e46963
to
58ea91d
Compare
This PR was extracted from #5452, to make reviewing easier as this ones just focus on changes to the blocks API.
Description
Before the migrate function received the existing attributes and returned the new attributes now, migrate receives an object with attributes and innerBlocks and returns another object with the migrated attributes & innerBlocks.
This allows, for example, to migrate an attribute to an innerBlock, or the contrary move an inner block to be an attribute.
These changes were used to migrate existing cover image blocks to the nested version.
How Has This Been Tested?
To test the cover image migration merge this changes in a temporary branch with the nesting in cover image PR.
Add a post with following code:
Open the post in the visual editor and verify no invalid block error appears and the cover image blocks and paragraphs were migrated.