Skip to content

Blocks: Fix BlockEdit hooks not working properly with context#6312

Merged
gziolo merged 1 commit intomasterfrom
fix/block-edit-hooks
Apr 23, 2018
Merged

Blocks: Fix BlockEdit hooks not working properly with context#6312
gziolo merged 1 commit intomasterfrom
fix/block-edit-hooks

Conversation

@gziolo
Copy link
Copy Markdown
Member

@gziolo gziolo commented Apr 20, 2018

Description

Addresses the comment shared by @phpbits:

I can't find the changes where the blocks.BlockEdit were disabled on blocks inside the columns. I thought it's this one, would you mind pointing me to the right direction?

I've created this plugin : http://wordpress.org/plugins/block-options/ and the options I've added are not showing on the inner columns anymore. Thanks!

When introducing BlockEditContext the handling of blocks.BlockEdit hook regressed for the nested blocks. It was caused by the fact that most of the filters wrap BlockEdit block with additional logic. This made nested blocks to use the parent context which had isSelected obviously set to false when editing the child block. I'm not sure why it worked for all other blocks, maybe because isSelected was set to true for backward compatibility (I changed to false since we migrated all core blocks). This PR extracts Editblock to apply filters directly to it and leavesBlockEdit` block responsible to handle or contexts.

How has this been tested?

npm run test
Manual tests. Make sure you can see Advanced section for the nested blocks.

Types of changes

Bug fix (non-breaking change which fixes an issue).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Blocks Overall functionality of blocks [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Apr 20, 2018
@gziolo gziolo added this to the 2.8 milestone Apr 20, 2018
@gziolo gziolo self-assigned this Apr 20, 2018
@gziolo gziolo requested review from a team and aduth April 20, 2018 10:31
@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Apr 20, 2018

const { Consumer, Provider } = createContext( {
isSelected: true,
isSelected: false,
Copy link
Copy Markdown
Member Author

@gziolo gziolo Apr 20, 2018

Choose a reason for hiding this comment

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

Should default to false since we migrated all core blocks to work with the block edit context.

Copy link
Copy Markdown
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

👌 looks good. I don't see any regressions when clicking through the demo post.

const Edit = blockType.edit || blockType.save;

// For backwards compatibility concerns adds a focus and setFocus prop
// These should be removed after some time (maybe when merging to Core)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment isn't relevant anymore.

Copy link
Copy Markdown
Member

@noisysocks noisysocks Apr 23, 2018

Choose a reason for hiding this comment

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

@gziolo: This // comment here can be deleted. I see now how 'This comment isn't relevant anymore' can be mistaken as 'Ignore this PR review comment' 😄

edit: Never mind, it's been removed in #6352!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I started #6352 just after merged this one because I realized this when pushing the button :)

All sorted out, thanks for catching 👍

const Component = blockType.edit || blockType.save;

// For backwards compatibility concerns adds a focus and setFocus prop
// These should be removed after some time (maybe when merging to Core)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unrelated, but maybe we should deprecate focus and setFocus more formally, e.g. calling deprecated and removing the functionality in two minor versions.

const componentProps = {
    ...props,
    className,
    get focus() {
        deprecated( ... );
        return isSelected ? {} : false;
    },
    setFocus() {
        deprecated( ... );
    },
};
return <Component { ...componentProps } />;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we didn't include those originally because there was no direct alternative. We did several releases since this change, I think we can just remove it on the next release or so.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will open a follow-up with those lines removed and scheduled to be included in 2.8 👍

@gziolo gziolo merged commit d96d79b into master Apr 23, 2018
@gziolo gziolo deleted the fix/block-edit-hooks branch April 23, 2018 10:02
@gziolo
Copy link
Copy Markdown
Member Author

gziolo commented Apr 23, 2018

@phpbits, your issue should be now resolved 😃

@phpbits
Copy link
Copy Markdown
Contributor

phpbits commented Apr 25, 2018

@gziolo Perfect! Waiting for the fixes to be uploaded on the repository :) I'm having issue with building the package locally.

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

Labels

[Feature] Blocks Overall functionality of blocks [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants