Cover block: Use supports flag for block alignment #10758
Conversation
ZebulanStanphill
changed the title
|
Does removing "align" as an attribute break validation of older block shapes? |
|
@chrisvanpatten No, because an |
earnjam
added
[Block] Cover
| @@ -81,6 +75,10 @@ export const settings = { | |||
|
|
|||
| attributes: blockAttributes, | |||
|
|
|||
| supports: { | |||
| align: [ 'left', 'center', 'right', 'wide', 'full' ], | |||
There was a problem hiding this comment.
Can we use align: true?
There was a problem hiding this comment.
I thought making it more explicit would be better in case any more alignments are added in the future, but I'm fine with it being either way.
There was a problem hiding this comment.
I'd prefer if we were consistent between blocks (I think we're using true elsewhere). Also, if we add a new alignment (or remove one), it will update automatically.
There was a problem hiding this comment.
Alright, I'll change it to true, then. 🙂
| @@ -34,9 +31,6 @@ const blockAttributes = { | |||
| url: { | |||
| type: 'string', | |||
| }, | |||
| align: { | |||
There was a problem hiding this comment.
We are removing align from blockAttributes variable. The deprecations rely on this variable so we need to add the attribute to the existing deprecations relying upon blockAttributes.
There was a problem hiding this comment.
The align hook actually adds the align attribute itself, so the deprecations should be unaffected as the resulting attribute set would be identical to the current one with align manually defined:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/hooks/align.js#L85
There was a problem hiding this comment.
Actually, I think I'll go test it at some point later today/tomorrow just to make sure it isn't actually necessary. If it turns out I was misremembering, I'll update this PR to fix the deprecations. Thanks for pointing this out.
There was a problem hiding this comment.
@jorgefilipecosta Alright, I just tested to see if aligned Cover blocks from Gutenberg 4.7.0 are preserved when applying my changes. The Cover blocks are indeed preserved properly, so it looks like this PR is ready to be merged.
There was a problem hiding this comment.
Hi @ZebulanStanphill thank you for your tests. Like you said blocks created in 4.7.0 are still valid, we are not causing an invalidation problem.
The problem we are causing with this changes is making very old deprecations with alignments not work correctly (these deprecations are not from 4.7 are from older versions like 2.0).
As a sample problem:
If on master we go to the code editor and paste:
<!-- wp:cover {"align":"center"} -->
<section class="wp-block-cover has-background-dim aligncenter"><h2></h2></section>
<!-- /wp:cover -->
The code gets migrated to:
<!-- wp:cover {"align":"center"} -->
<div class="wp-block-cover has-background-dim aligncenter"></div>
<!-- /wp:cover -->
Which is correct, we have an empty cover image block with center alignment.
On this branch, if we paste the same sample the code gets migrated to:
<!-- wp:cover {"className":"aligncenter"} -->
<div class="wp-block-cover has-background-dim aligncenter"></div>
<!-- /wp:cover -->
Which is not correct the aligment was not recognized and the block was treated as a block with a custom className.
There was a problem hiding this comment.
Thanks for pointing that out. I have updated the PR to fix this problem. 🙂
|
@jorgefilipecosta @youknowriad I have updated the PR to use |
|
Thanks for the update :) |
Description
This PR updates the Cover block to use
supports: { align }to add block alignment support, rather than the more manual method currently used inmaster. This is essentially just code optimization. There should be no functional changes frommaster.Fixes: #12333
Related PRs