Address deprecation issues from Buttons flex layout PR. #36192
Merged
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
|
Size Change: +2 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
7 tasks
ntsekouras
reviewed
Nov 4, 2021
There was a problem hiding this comment.
Thanks for following with this @tellthemachines 👍 I've left a couple of comments.
| @@ -29,6 +29,8 @@ const migrateWithLayout = ( attributes ) => { | |||
| orientation: orientation || 'horizontal', | |||
| }, | |||
| } ); | |||
| delete updatedAttributes.contentJustification; | |||
There was a problem hiding this comment.
I think we can simplify in the destructuring above.
`const { contentJustification, orientation, ...updatedAttributes } = attributes;
Also we might want only to add layout's justifyContent or orientation props only if the respective attribute exists so as to get the defaults from flex.layout.
| @@ -57,7 +59,8 @@ const deprecated = [ | |||
| }, | |||
| }, | |||
| }, | |||
| isEligible: ( { layout } ) => ! layout, | |||
| isEligible: ( { contentJustification, orientation } ) => | |||
| !! contentJustification || !! orientation, | |||
There was a problem hiding this comment.
I still think we shouldn't have isEligible here (also commented about the difference with social-links).
This check is handled inside migrateWithLayout, no? Have you run into some problems without it?
There was a problem hiding this comment.
I did try removing it, but migrateWithLayout doesn't get called unless we have isEligible here. And then trying to edit the layout for existing blocks has unpredictable results.
There was a problem hiding this comment.
This needs some investigation, but it seems this might be related to the other deprecation that uses isEligible. For those we would still need to add the layout prop if not exists.
There was a problem hiding this comment.
Could you tell me more about why we shouldn't have isEligible here? As far as I can tell, if we don't use it, the migration won't run.
The block handbook says "A deprecation will be tried if the current state of a parsed block is invalid, or if the deprecation defines an isEligible function that returns true".
In this case, a block with contentJustification or orientation and a bunch of extra classnames isn't invalid; what happens when it runs through the current save function is the attributes get stripped and the classnames get added as attributes. So, if we want the migration to work (which we do, otherwise it'll be impossible to change the layout of an existing block), we have to use isEligible. Or am I missing something here?
this might be related to the other deprecation that uses isEligible. For those we would still need to add the layout prop if not exists.
I did make sure to add migrateWithLayout to the older deprecation too. Is that what you meant?
There was a problem hiding this comment.
In this case, a block with contentJustification or orientation and a bunch of extra classnames isn't invalid;
This seems to be right, yes. Deprecations can be super confusing at times 😄
this might be related to the other deprecation that uses isEligible.
I got confused whether we should add the layout prop in the other deprecation but it seems fine, as we use the default layout for the block.
ntsekouras
approved these changes
Nov 9, 2021
There was a problem hiding this comment.
Thanks @tellthemachines ! As I said deprecations can be very confusing at time 😄
|
Yeah, deprecations are hard 😅 Thanks for updating the fixtures @ntsekouras ! |
e745230
to
3d9445d
Compare
noisysocks
pushed a commit
that referenced
this pull request
Nov 10, 2021
* Address deprecation issues from Buttons flex layout PR. * Simplify migration logic. * update fixtures Co-authored-by: ntsekouras <[email protected]>
andrewserong
pushed a commit
that referenced
this pull request
Nov 10, 2021
* Address deprecation issues from Buttons flex layout PR. * Simplify migration logic. * update fixtures Co-authored-by: ntsekouras <[email protected]>
|
Cherry picked into the Gutenberg 11.9 release in: 4b30711 |
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.
Description
Addresses feedback from #35819.
Buttons deprecation is fixed so that layout attributes are only migrated if they are explicitly set.
Also removes unnecessary BlockControls from Buttons edit function.
How has this been tested?
Test by trying out different combinations of Buttons layouts, saving and refreshing to make sure they work as expected.
Test the deprecations by generating some deprecated markup in an older version of the editor, pasting it in a post and saving: the markup should be updated on save, unless there are no layout attributes, in which case nothing should change.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).