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.

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

Block Library: Add width attribute for resizable Column blocks #15499

Merged
merged 12 commits into from
May 12, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 5 additions & 0 deletions packages/block-library/src/column/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
"attributes": {
"verticalAlignment": {
"type": "string"
},
"width": {
"type": "number",
"min": 0,
"max": 100
}
}
}
102 changes: 82 additions & 20 deletions packages/block-library/src/column/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,52 +6,114 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { InnerBlocks, BlockControls, BlockVerticalAlignmentToolbar } from '@wordpress/block-editor';
import {
InnerBlocks,
BlockControls,
BlockVerticalAlignmentToolbar,
InspectorControls,
} from '@wordpress/block-editor';
import { PanelBody, RangeControl } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

const ColumnEdit = ( { attributes, updateAlignment } ) => {
const { verticalAlignment } = attributes;
function ColumnEdit( {
attributes,
updateAlignment,
updateWidth,
hasChildBlocks,
} ) {
const { verticalAlignment, width } = attributes;

const classes = classnames( 'block-core-columns', {
[ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
} );

const onChange = ( alignment ) => updateAlignment( alignment );

return (
<div className={ classes }>
<BlockControls>
<BlockVerticalAlignmentToolbar
onChange={ onChange }
onChange={ updateAlignment }
value={ verticalAlignment }
/>
</BlockControls>
<InnerBlocks templateLock={ false } />
<InspectorControls>
<PanelBody title={ __( 'Column Settings' ) }>
<RangeControl
label={ __( 'Width' ) }
value={ width }
onChange={ updateWidth }
min={ 0 }
max={ 100 }
required
/>
</PanelBody>
</InspectorControls>
<InnerBlocks
templateLock={ false }
renderAppender={ (
hasChildBlocks ?
undefined :
() => <InnerBlocks.ButtonBlockAppender />
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you why the sibling inserter is not shown when there's a single column with a single block? It feels like a separate issue but it's made more obvious with this change

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you why the sibling inserter is not shown when there's a single column with a single block? It feels like a separate issue but it's made more obvious with this change

I think it's the same as the fact that there is no sibling inserter shown at the end of the block list, only the "default appender". The column is effectively the same as the top-level block list.

However, I don't necessary agree that the sibling inserter shouldn't be shown after the last block in a block list. I think there may have been an issue about this at some point?

) }
/>
</div>
);
};
}

export default compose(
withSelect( ( select, { clientId } ) => {
const { getBlockRootClientId } = select( 'core/editor' );
withSelect( ( select, ownProps ) => {
const { clientId } = ownProps;
const { getBlockOrder } = select( 'core/block-editor' );

return {
parentColumnsBlockClientId: getBlockRootClientId( clientId ),
hasChildBlocks: getBlockOrder( clientId ).length > 0,
};
} ),
withDispatch( ( dispatch, { clientId, parentColumnsBlockClientId } ) => {
withDispatch( ( dispatch, ownProps, registry ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Any reason you didn't destructure registry

Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: Any reason you didn't destructure registry

I'm personally not a fan of destructuring in an arguments signature, since it loses context of what it is that is being destructured. To a lesser degree, I find it hard to visually distinguish between proper arguments, and properties destructured (attention to the { and }).

return {
updateAlignment( alignment ) {
// Update self...
dispatch( 'core/editor' ).updateBlockAttributes( clientId, {
verticalAlignment: alignment,
} );
updateAlignment( verticalAlignment ) {
const { clientId, setAttributes } = ownProps;
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
const { getBlockRootClientId } = registry.select( 'core/block-editor' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice little perf tweak

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice little perf tweak

Thanks for reminding: I'd meant to reference some commentary at #13899 (comment) which had led to some of this general (but related/required) refactoring.


// Update own alignment.
setAttributes( { verticalAlignment } );

// Reset Parent Columns Block
dispatch( 'core/editor' ).updateBlockAttributes( parentColumnsBlockClientId, {
verticalAlignment: null,
} );
const rootClientId = getBlockRootClientId( clientId );
updateBlockAttributes( rootClientId, { verticalAlignment: null } );
},
updateWidth( width ) {
const { clientId, attributes, setAttributes } = ownProps;
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
const {
getBlockRootClientId,
getBlockOrder,
getBlockAttributes,
} = registry.select( 'core/block-editor' );

// Update own width.
setAttributes( { width } );

// Constrain or expand siblings to account for gain or loss of
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the computations here could be extracted to a unit-testable pure function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if the computations here could be extracted to a unit-testable pure function.

Yeah, I expect there's a fair bit of clean-up which could be done here 😅 Perhaps even some reusability between redistribution logic between the individual Column and its parent Columns block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if the computations here could be extracted to a unit-testable pure function.

Still generally more complex than I'd have liked, but improved refactoring and tests in 6880cfa.

// total columns area.
const rootClientId = getBlockRootClientId( clientId );
const columnClientIds = getBlockOrder( rootClientId );
const { width: previousWidth = 100 / columnClientIds.length } = attributes;
const index = columnClientIds.indexOf( clientId );
const isLastColumn = index === columnClientIds.length - 1;
const increment = isLastColumn ? -1 : 1;
const endIndex = isLastColumn ? 0 : columnClientIds.length - 1;
const adjustment = ( previousWidth - width ) / Math.abs( index - endIndex );

for ( let i = index + increment; i - increment !== endIndex; i += increment ) {
const columnClientId = columnClientIds[ i ];
const { width: columnWidth = 100 / columnClientIds.length } = getBlockAttributes( columnClientId );
updateBlockAttributes( columnClientId, {
width: columnWidth + adjustment,
} );
}
},
};
} )
Expand Down
10 changes: 10 additions & 0 deletions packages/block-library/src/column/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ export const settings = {
reusable: false,
html: false,
},
getEditWrapperProps( attributes ) {
const { width } = attributes;
if ( Number.isFinite( width ) ) {
return {
style: {
flexBasis: width + '%',
},
};
}
},
edit,
save,
};
Expand Down
10 changes: 8 additions & 2 deletions packages/block-library/src/column/save.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,19 @@ import classnames from 'classnames';
import { InnerBlocks } from '@wordpress/block-editor';

export default function save( { attributes } ) {
const { verticalAlignment } = attributes;
const { verticalAlignment, width } = attributes;

const wrapperClasses = classnames( {
[ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
} );

let style;
if ( Number.isFinite( width ) ) {
style = { flexBasis: width + '%' };
Copy link
Contributor

Choose a reason for hiding this comment

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

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

Good catch! It is not.

I will need to filter this set, and follow-up with a Trac ticket.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/kses.php?rev=45242#L2057

Copy link
Member Author

Choose a reason for hiding this comment

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

is this kses allowed (just to make sure we check as all inline style properties are not allowed)?

Added in 5bb641c

}

return (
<div className={ wrapperClasses }>
<div className={ wrapperClasses } style={ style }>
<InnerBlocks.Content />
</div>
);
Expand Down
148 changes: 96 additions & 52 deletions packages/block-library/src/columns/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
* External dependencies
*/
import classnames from 'classnames';
import { last } from 'lodash';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { compose } from '@wordpress/compose';
import {
PanelBody,
RangeControl,
Expand All @@ -18,7 +18,8 @@ import {
BlockControls,
BlockVerticalAlignmentToolbar,
} from '@wordpress/block-editor';
import { withSelect, withDispatch } from '@wordpress/data';
import { withDispatch } from '@wordpress/data';
import { createBlock } from '@wordpress/blocks';

/**
* Internal dependencies
Expand All @@ -36,38 +37,34 @@ import { getColumnsTemplate } from './utils';
*/
const ALLOWED_BLOCKS = [ 'core/column' ];

export const ColumnsEdit = function( { attributes, setAttributes, className, updateAlignment } ) {
export function ColumnsEdit( {
attributes,
className,
updateAlignment,
updateColumns,
} ) {
const { columns, verticalAlignment } = attributes;

const classes = classnames( className, `has-${ columns }-columns`, {
[ `are-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
} );

const onChange = ( alignment ) => {
// Update all the (immediate) child Column Blocks
updateAlignment( alignment );
};

return (
<>
<InspectorControls>
<PanelBody>
<RangeControl
label={ __( 'Columns' ) }
value={ columns }
onChange={ ( nextColumns ) => {
setAttributes( {
columns: nextColumns,
} );
} }
onChange={ updateColumns }
min={ 2 }
max={ 6 }
/>
</PanelBody>
</InspectorControls>
<BlockControls>
<BlockVerticalAlignmentToolbar
onChange={ onChange }
onChange={ updateAlignment }
value={ verticalAlignment }
/>
</BlockControls>
Expand All @@ -79,45 +76,92 @@ export const ColumnsEdit = function( { attributes, setAttributes, className, upd
</div>
</>
);
};
}

export default withDispatch( ( dispatch, ownProps, registry ) => ( {
/**
* Update all child Column blocks with a new vertical alignment setting
* based on whatever alignment is passed in. This allows change to parent
* to overide anything set on a individual column basis.
*
* @param {string} verticalAlignment the vertical alignment setting
*/
updateAlignment( verticalAlignment ) {
const { clientId, setAttributes } = ownProps;
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
const { getBlockOrder } = registry.select( 'core/block-editor' );

const DEFAULT_EMPTY_ARRAY = [];
// Update own alignment.
setAttributes( { verticalAlignment } );

// Update all child Column Blocks to match
const innerBlockClientIds = getBlockOrder( clientId );
innerBlockClientIds.forEach( ( innerBlockClientId ) => {
updateBlockAttributes( innerBlockClientId, {
verticalAlignment,
} );
} );
},

export default compose(
/**
* Selects the child column Blocks for this parent Column
* Updates the column count, including necessary revisions to child Column
* blocks to grant required or redistribute available space.
*
* @param {number} columns New column count.
*/
withSelect( ( select, { clientId } ) => {
const { getBlocksByClientId } = select( 'core/editor' );
const block = getBlocksByClientId( clientId )[ 0 ];

return {
childColumns: block ? block.innerBlocks : DEFAULT_EMPTY_ARRAY,
};
} ),
withDispatch( ( dispatch, { clientId, childColumns } ) => {
return {
/**
* Update all child column Blocks with a new
* vertical alignment setting based on whatever
* alignment is passed in. This allows change to parent
* to overide anything set on a individual column basis
*
* @param {string} alignment the vertical alignment setting
*/
updateAlignment( alignment ) {
// Update self...
dispatch( 'core/editor' ).updateBlockAttributes( clientId, {
verticalAlignment: alignment,
} );

// Update all child Column Blocks to match
childColumns.forEach( ( childColumn ) => {
dispatch( 'core/editor' ).updateBlockAttributes( childColumn.clientId, {
verticalAlignment: alignment,
} );
} );
},
};
} ),
)( ColumnsEdit );
updateColumns( columns ) {
const { clientId, setAttributes, attributes } = ownProps;
const { replaceInnerBlocks } = dispatch( 'core/block-editor' );
const { getBlocks } = registry.select( 'core/block-editor' );

// Update columns count.
setAttributes( { columns } );

let innerBlocks = getBlocks( clientId );
const hasExplicitColumnWidths = innerBlocks.some( ( innerBlock ) => (
innerBlock.attributes.width !== undefined
) );

let newOrRemovedColumnWidth;
if ( ! hasExplicitColumnWidths ) {
return;
}

// Redistribute available width for existing inner blocks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, I wonder if this could be unit testable

const { columns: previousColumns } = attributes;
const isAddingColumn = columns > previousColumns;

if ( isAddingColumn ) {
// If adding a new column, assign width to the new column equal to
// as if it were `1 / columns` of the total available space.
newOrRemovedColumnWidth = ( 100 / columns );
} else {
// The removed column will be the last of the inner blocks.
newOrRemovedColumnWidth = last( innerBlocks ).attributes.width || ( 100 / previousColumns );
}

const adjustment = newOrRemovedColumnWidth / ( isAddingColumn ? -1 * previousColumns : columns );
innerBlocks = innerBlocks.map( ( innerBlock ) => {
const { width: columnWidth = ( 100 / previousColumns ) } = innerBlock.attributes;
return {
...innerBlock,
attributes: {
...innerBlocks.attributes,
width: parseFloat( ( columnWidth + adjustment ).toFixed( 2 ) ),
},
};
} );

// Explicitly manage the new column block, since template would not
// account for the explicitly assigned width.
if ( isAddingColumn ) {
const block = createBlock( 'core/column', {
width: parseFloat( newOrRemovedColumnWidth.toFixed( 2 ) ),
} );

innerBlocks = [ ...innerBlocks, block ];
}

replaceInnerBlocks( clientId, innerBlocks, false );
},
} ) )( ColumnsEdit );
Loading