Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cover block: Use supports flag for block alignment #10758

Merged
merged 1 commit into from
Jan 7, 2019
Merged

Cover block: Use supports flag for block alignment #10758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 10 additions & 21 deletions packages/block-library/src/cover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { compose } from '@wordpress/compose';
import {
BlockControls,
InspectorControls,
BlockAlignmentToolbar,
MediaPlaceholder,
MediaUpload,
MediaUploadCheck,
Expand All @@ -34,8 +33,6 @@ import {
getColorClassName,
} from '@wordpress/editor';

const validAlignments = [ 'left', 'center', 'right', 'wide', 'full' ];

const blockAttributes = {
title: {
type: 'string',
Expand All @@ -45,9 +42,6 @@ const blockAttributes = {
url: {
type: 'string',
},
align: {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I have updated the PR to fix this problem. 🙂

type: 'string',
},
contentAlign: {
type: 'string',
default: 'center',
Expand Down Expand Up @@ -92,6 +86,10 @@ export const settings = {

attributes: blockAttributes,

supports: {
align: true,
},

transforms: {
from: [
{
Expand Down Expand Up @@ -168,20 +166,12 @@ export const settings = {
],
},

getEditWrapperProps( attributes ) {
const { align } = attributes;
if ( -1 !== validAlignments.indexOf( align ) ) {
return { 'data-align': align };
}
},

edit: compose( [
withColors( { overlayColor: 'background-color' } ),
withNotices,
] )(
( { attributes, setAttributes, isSelected, className, noticeOperations, noticeUI, overlayColor, setOverlayColor } ) => {
const {
align,
backgroundType,
contentAlign,
dimRatio,
Expand All @@ -190,7 +180,6 @@ export const settings = {
title,
url,
} = attributes;
const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
const onSelectMedia = ( media ) => {
if ( ! media || ! media.url ) {
setAttributes( { url: undefined, id: undefined } );
Expand Down Expand Up @@ -238,10 +227,6 @@ export const settings = {
const controls = (
<Fragment>
<BlockControls>
<BlockAlignmentToolbar
value={ align }
onChange={ updateAlignment }
/>
{ !! url && (
<Fragment>
<AlignmentToolbar
Expand Down Expand Up @@ -381,7 +366,6 @@ export const settings = {

save( { attributes } ) {
const {
align,
backgroundType,
contentAlign,
customOverlayColor,
Expand All @@ -407,7 +391,6 @@ export const settings = {
'has-parallax': hasParallax,
[ `has-${ contentAlign }-content` ]: contentAlign !== 'center',
},
align ? `align${ align }` : null,
);

return (
Expand All @@ -429,6 +412,9 @@ export const settings = {
deprecated: [ {
attributes: {
...blockAttributes,
align: {
type: 'string',
},
},

supports: {
Expand Down Expand Up @@ -466,6 +452,9 @@ export const settings = {
}, {
attributes: {
...blockAttributes,
align: {
type: 'string',
},
title: {
type: 'string',
source: 'html',
Expand Down