Categories block: use supports flag for alignment + wide alignment support #10666
Conversation
ZebulanStanphill
changed the title
|
I made a mistake on the PR... will fix in a moment. |
|
Okay, I've fixed the issue. This PR is now ready for review. @chrisvanpatten |
| 'attributes' => array( | ||
| 'align' => array( | ||
| 'type' => 'string', | ||
| 'enum' => array( 'left', 'center', 'right', 'wide', 'full' ), |
There was a problem hiding this comment.
Notably, the values passed to enum have no effect on what is allowed in the editor (removing full does not make the full option disappear from the block toolbar), but they do have an effect on what classes end up getting applied on the front-end (removing full causes the alignfull class to not be applied on the front-end).
There was a problem hiding this comment.
The JS and the PHP don't communicate about this right now, but one thing to note is that by specifying the enum here, but not explicitly listing out the alignments in the JS supports.align, it could be possible that new alignment options are added to the editor but aren't supported here. I think that's not a huge deal, but worth noting.
|
@ZebulanStanphill I know you're keen to get consistency between blocks but it'll be trickier to merge this because of how big the changes are, without e2e tests. Is there a reason you can't just update the block to use I wish we had more time to explore greater consistency between the server rendered blocks but because we're under the gun in terms of timeline I think it'd be much safer to simply update the alignment without making other potentially breaking changes. |
| @@ -19,35 +19,11 @@ export const settings = { | |||
|
|
|||
| category: 'widgets', | |||
|
|
|||
| attributes: { | |||
There was a problem hiding this comment.
To keep us on track here I'd really recommend preserving the definitions in JS — it keeps the change surface minimised as we careen toward code freezes.
There was a problem hiding this comment.
@chrisvanpatten If the definitions remain in JS but are removed from PHP, I can't stop invalid classes from being applied. Should I define the attributes in both JS and PHP?
There was a problem hiding this comment.
The JS should already ensure that only valid alignments are set to the align attribute. It doesn't need to be reinforced in PHP (or to be more specific, we aren't currently enforcing it on PHP in this block and if an 'invalid' alignment somehow made it through to PHP anyway it isn't a site-breaking issue).
ZebulanStanphill
changed the title
|
@chrisvanpatten Addressed feedback. Note that, using the Code Editor, set the However, since this can only really happen if the user is intentionally trying to mess up the alignment classes, and since (I think) this behavior also occurs on |
|
Test failures seemed unrelated. Squashed and rebased. |
|
Tests are still failing and I can't figure out why. 😕 It seems to have something to do with #10682. |
|
@ZebulanStanphill Tests are failing in a bunch of PRs; I can restart Travis a little later once the problem is fixed. |
|
@chrisvanpatten Tests are no longer failing on this, #10706, or #10707. 🙂 |
|
@chrisvanpatten I have rebased this PR to resolve conflicts with |
ZebulanStanphill
deleted the
update/categories-block-move-attributes-to-php-2
branch
Description
Previously #10635.
This PR changes the code for the Categories block to use the
supportsflag. It also adds support for thewidealignment to the Categories block, which strangely was lacking support for it despite already supporting thefullalignment.How has this been tested?
I opened a post with an existing Categories block and checked to make sure setting alignments worked and applied the right classes in the editor and front-end.
Types of changes
supports: { align: true }to add block alignment support. Note that thealignattribute is still defined in PHP, as this is necessary for the alignments to work on the front-end.widealignment. (The block already supported thefullalignment.)