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

Inner Blocks: Respecting locking in display of default block appender #7732

Merged
merged 3 commits into from Jul 5, 2018
Merged

Inner Blocks: Respecting locking in display of default block appender #7732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a03e97c
Block List: Avoid rendering default appender if locked
aduth Jul 5, 2018
91e883e
Inner Blocks: Prepare locking for initial InnerBlocks render
aduth Jul 5, 2018
dc48c3d
Inner Blocks: Avoid synchronizing template on mismatch update
aduth Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 15 additions & 8 deletions editor/components/block-list/layout.js
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ import 'element-closest';
*/
import { Component, compose } from '@wordpress/element';
import { withSelect, withDispatch } from '@wordpress/data';
import { getDefaultBlockName } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -193,6 +194,7 @@ class BlockListLayout extends Component {
layout,
isGroupedByLayout,
rootUID,
canInsertDefaultBlock,
} = this.props;

let defaultLayout;
Expand Down Expand Up @@ -220,34 +222,39 @@ class BlockListLayout extends Component {
isLast={ blockIndex === blockUIDs.length - 1 }
/>
) ) }
<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<DefaultBlockAppender
rootUID={ rootUID }
lastBlockUID={ last( blockUIDs ) }
layout={ defaultLayout }
/>
</IgnoreNestedEvents>
{ canInsertDefaultBlock && (
<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<DefaultBlockAppender
rootUID={ rootUID }
lastBlockUID={ last( blockUIDs ) }
layout={ defaultLayout }
/>
</IgnoreNestedEvents>
) }
</div>
);
}
}

export default compose( [
withSelect( ( select ) => {
withSelect( ( select, ownProps ) => {
const {
isSelectionEnabled,
isMultiSelecting,
getMultiSelectedBlocksStartUid,
getMultiSelectedBlocksEndUid,
getBlockSelectionStart,
canInsertBlockType,
} = select( 'core/editor' );
const { rootUID } = ownProps;

return {
selectionStart: getMultiSelectedBlocksStartUid(),
selectionEnd: getMultiSelectedBlocksEndUid(),
selectionStartUID: getBlockSelectionStart(),
isSelectionEnabled: isSelectionEnabled(),
isMultiSelecting: isMultiSelecting(),
canInsertDefaultBlock: canInsertBlockType( getDefaultBlockName(), rootUID ),
};
} ),
withDispatch( ( dispatch ) => {
Expand Down
16 changes: 8 additions & 8 deletions editor/components/inner-blocks/index.js
View file
Open in desktop
Expand Up @@ -22,23 +22,23 @@ import BlockList from '../block-list';
import { withBlockEditContext } from '../block-edit/context';

class InnerBlocks extends Component {
componentDidMount() {
constructor() {
super( ...arguments );

this.updateNestedSettings();
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 don't really like that we rely on specific component lifecycle to effect nested locking.

}

componentDidMount() {
this.synchronizeBlocksWithTemplate();
}

componentDidUpdate( prevProps ) {
const { template, block } = this.props;
const { template } = this.props;

this.updateNestedSettings();

const hasTemplateChanged = ! isEqual( template, prevProps.template );
Copy link
Member

Choose a reason for hiding this comment

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

Here we compare using equal while on allowedBlocks we compare by reference. We should probably decide on one of the approaches while comparing by reference is more performant this comparison is more developer friendly.

Copy link
Member Author

@aduth aduth Jul 5, 2018 •
edited

Choose a reason for hiding this comment

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

I agree we need consistency here. Not sure performance is a fair measurement in this case though, since in almost all cases allowedBlocks will be referentially unequal, since with usage like <InnerBlocks allowed={ [ 'core/paragraph' ] } /> we are passing a new array reference each time, thus forcing the update even though the allowed blocks have not really changed.

Copy link
Member

Choose a reason for hiding this comment

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

Our docs already contain the samples using a constant.

const ALLOWED_BLOCKS = [ 'core/paragraph' ];
...
<InnerBlocks allowed={ ALLOWED_BLOCKS } />

For dynamic cases, we can save a reference in the state.
But yes is more complex for developers and we may get rerenders on external blocks.

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 think we may be better off doing a deep equality check on the allowed blocks. It's not too much more effort to dive into the array of strings, and avoids a much more concerning overhead if the developer doesn't assign allowed blocks as a constant reference (which itself is overhead in its own right). I'll plan to open a separate pull request proposing this.

const isTemplateInnerBlockMismatch = (
template &&
block.innerBlocks.length !== template.length
);

if ( hasTemplateChanged || isTemplateInnerBlockMismatch ) {
if ( hasTemplateChanged ) {
this.synchronizeBlocksWithTemplate();
}
}
Expand Down