-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Cover: Add missing align attr to deprecation #28796
Conversation
Size Change: +213 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
The Cover align attribute may be lost during deprecation handling as can be observed in the deprecation test. The attribute should be parsed and preserved when the block is upgraded.
a8db4d1
to
e929bfe
Compare
cc/ @nosolosw, we might want to include this in a 9.9 point release 🙂 |
align: { | ||
type: 'string', | ||
}, |
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 not entirely sure this is the correct fix 🤔
The 4th deprecation actually uses a supports
declaration for align
:
gutenberg/packages/block-library/src/cover/deprecated.js
Lines 419 to 421 in 8a39954
supports: { | |
align: true, | |
}, |
So maybe we need to copy that to all more recent deprecations? 🤔
cc/ @gziolo since AFAICT you wrote that deprecation.
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.
Did I? It might be as well moving code 😅
The best way to verify is to copy the old version in the post and check if it properly converts.
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 not entirely sure this is the correct fix 🤔
The 4th deprecation actually uses a
supports
declaration foralign
:gutenberg/packages/block-library/src/cover/deprecated.js
Lines 419 to 421 in 8a39954
supports: { align: true, }, So maybe we need to copy that to all more recent deprecations? 🤔
cc/ @gziolo since AFAICT you wrote that deprecation.
Followed this advice in 0183f5e. Had to regenerate the fixtures but it looks better now and works well for me! Thanks! ❤️
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! Thanks all! ❤️
Adding a minimal, manual test case:
|
Description
The Cover align attribute may be lost during deprecation handling as can be observed in the deprecation test. The attribute should be parsed and preserved when the block is upgraded.
Types of changes
Checklist: