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

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

Merged
merged 3 commits into from Jul 5, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented Jul 5, 2018

Alternative to #7723

This pull request seeks to resolve an issue where inserting a block which had full locking on its template would cause a default block to be appended because the DefaultBlockAppender's rendering was not conditional based on the locking settings of its layout context. This was only not obvious previously because we had a condition in the InnerBlocks to allow the template to synchronize itself on the basis of block count mismatch, so the templated blocks would be reset to its intended form after the default block was appended.

Example:

Insert Columns
Template has 2 "Column" blocks.
Default appender would be focused by BlockListBlock's focusTabbable and a new paragraph wrongly inserted into "Columns" block.
Previously: InnerBlocks would re-sync its template to delete the paragraph block.
Now: The paragraph block is never inserted to begin with.

Testing instructions:

Repeat testing instructions from #7723
Verify that template is respected when inserting Columns block.
Ensure end-to-end tests pass:

npm run test-e2e
componentDidMount() {
constructor() {
super( ...arguments );
this.updateNestedSettings();

This comment has been minimized.

@aduth

aduth Jul 5, 2018

Member

I don't really like that we rely on specific component lifecycle to effect nested locking.

this.synchronizeBlocksWithTemplate();
}
componentDidUpdate( prevProps ) {
const { template, block } = this.props;
const { template } = this.props;
this.updateNestedSettings();
const hasTemplateChanged = ! isEqual( template, prevProps.template );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 5, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Jul 5, 2018

Member

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 5, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Jul 5, 2018

Member

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.

@jorgefilipecosta

I like the changes here. They simplify the logic and solve the problem described in #7723 in a simpler way.

In my tests using https://gist.github.com/jorgefilipecosta/2bac096fe3be7f6ff0de7452292161cb things worked well.

And I verified on the dom for columns that we don't have a default appender while on master we have.

@aduth aduth merged commit dd6d2c2 into master Jul 5, 2018

2 checks passed

codecov/project 46.42% (-0.01%) compared to 622bbfb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/inner-blocks-template branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment