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 all 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
26 changes: 26 additions & 0 deletions lib/compat.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
/**
* Temporary compatibility shims for features present in Gutenberg, pending
* upstream commit to the WordPress core source repository. Functions here
* exist only as long as necessary for corresponding WordPress support, and
* each should be associated with a Trac ticket.
*
* @package gutenberg
*/

/**
* Filters allowed CSS attributes to include `flex-basis`, included in saved
* markup of the Column block.
*
* @since 5.7.0
*
* @param string[] $attr Array of allowed CSS attributes.
*
* @return string[] Filtered array of allowed CSS attributes.
*/
function gutenberg_safe_style_css_column_flex_basis( $attr ) {
$attr[] = 'flex-basis';

return $attr;
}
add_filter( 'safe_style_css', 'gutenberg_safe_style_css_column_flex_basis' );
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
require dirname( __FILE__ ) . '/rest-api.php';
}

require dirname( __FILE__ ) . '/compat.php';
require dirname( __FILE__ ) . '/blocks.php';
require dirname( __FILE__ ) . '/client-assets.php';
require dirname( __FILE__ ) . '/demo.php';
Expand Down
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
}
}
}
115 changes: 96 additions & 19 deletions packages/block-library/src/column/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,132 @@
* External dependencies
*/
import classnames from 'classnames';
import { forEach, find, difference } from 'lodash';

/**
* 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;
/**
* Internal dependencies
*/
import {
toWidthPrecision,
getTotalColumnsWidth,
getColumnWidths,
getAdjacentBlocks,
getRedistributedColumnWidths,
} from '../columns/utils';

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={ __( 'Percentage width' ) }
value={ width || '' }
onChange={ updateWidth }
min={ 0 }
max={ 100 }
required
allowReset
/>
</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 } = ownProps;
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
const { getBlockRootClientId, getBlocks } = registry.select( 'core/block-editor' );

// 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 columns = getBlocks( getBlockRootClientId( clientId ) );
const adjacentColumns = getAdjacentBlocks( columns, clientId );

// The occupied width is calculated as the sum of the new width
// and the total width of blocks _not_ in the adjacent set.
const occupiedWidth = width + getTotalColumnsWidth(
difference( columns, [
find( columns, { clientId } ),
...adjacentColumns,
] )
);

// Compute _all_ next column widths, in case the updated column
// is in the middle of a set of columns which don't yet have
// any explicit widths assigned (include updates to those not
// part of the adjacent blocks).
const nextColumnWidths = {
...getColumnWidths( columns, columns.length ),
[ clientId ]: toWidthPrecision( width ),
...getRedistributedColumnWidths( adjacentColumns, 100 - occupiedWidth, columns.length ),
};

forEach( nextColumnWidths, ( nextColumnWidth, columnClientId ) => {
updateBlockAttributes( columnClientId, { width: nextColumnWidth } );
} );
},
};
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
Loading