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 List: Remove createInnerBlockList utility / context #7192

Merged
merged 1 commit into from Jun 20, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Jun 6, 2018

Closes #6943

This pull request seeks to remove the createInnerBlockList utility function and block context property which, as described in #6943, was introduced at a time when the InnerBlocks component could not depend on components in Editor.

This should be a simplification of block context, potentially with some performance and/or memory improvements, as an intermediary component is no longer created and instead the block context contains simpler values: a uid which already existed, a renderBlockMenu function which can hopefully be eliminated at some point. Previously we would create, cache, and pass through a custom component definition for each block, even if that block did not support nesting.

Testing instructions:

Verify there are no regressions to the display an interaction of nested blocks. Of note:

  • Previewing shared nested blocks in the inserter should work as expected
  • renderBlockMenu (what's this for again, @youknowriad ?)
return {
...prevState,

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

As part of #6419 (ead8dc5), I'm guessing this was previously trying to avoid clobbering the focusedElement and setFocusedElement assigned in constructor. From my testing, this doesn't happen: The return value from getDerivedStateFromProps is applied as patch (same as setState):

Demo: https://codepen.io/aduth/pen/YvGyag

cc @gziolo

This comment has been minimized.

@gziolo

gziolo Jun 6, 2018

Member

I will test it tomorrow. I think it was introduced before and was trying to prevent state updates when props didn’t change by returning null. Maybe your proposal works the same, it would be awesome 😃

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

I found my commit which I used to test BlockEdit updates: 909844f. Great refactoring, everything works as expected. I didn't discover any regression and the code is much cleaner 💯

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 7, 2018

renderBlockMenu (what's this for again, @youknowriad ?)

This is a way to extend the BlockMenu in the edit-post module. I think we can probably replace it by a Slot. I believe even plugin authors could be interested to hook into this menu. I'll give it a shot in a separate PR.

@youknowriad

I had this in mind as well :) Nice refactoring 👍

@@ -5,6 +5,7 @@ export { default as AlignmentToolbar } from './alignment-toolbar';
export { default as BlockAlignmentToolbar } from './block-alignment-toolbar';
export { default as BlockControls } from './block-controls';
export { default as BlockEdit } from './block-edit';
export { withBlockEditContext } from './block-edit/context';

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

I'd prefer if we don't expose this one to avoid confusion with Block edit props for block authors. Hopefully we can get rid of createInnerBlockList

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

Well, it would allow me to refactor Gallery and Image to remove focus when an image is clicked so I'm not that against exposing it :)

However, I think we agreed that we should avoid exposing this context if possible. If there is a better way to do it, then let's keep this context internal as long as necessary.

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

If we merge #7199, we shouldn't need to export it. It's used exclusively by the shared block in its rendering of <BlockEdit /> which needs renderBlockMenu. If we can make it work with just uid (already passed as a prop to block's edit), then it can be avoided.

This comment has been minimized.

@gziolo

gziolo Jun 8, 2018

Member

Let’s merge #7199 in that case 🎉

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 7, 2018

I had this in mind as well :) Nice refactoring 👍

Welcome to the club. I tried to keep away myself from it because of the complexity :)

}
InnerBlocks = compose( [
withContext( 'BlockList' )(),
withContext( 'uid' )(),
withBlockEditContext( ( context ) => pick( context, [ 'uid', 'renderBlockMenu' ] ) ),

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

In #7199 @youknowriad proposes how to get rid of renderBlockMenu from the context. Would it be possible to pass down uid to InnerBlocks instead to avoid exposing it through BlockEditContext?

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

In #7199 @youknowriad proposes how to get rid of renderBlockMenu from the context. Would it be possible to pass down uid to InnerBlocks instead to avoid exposing it through BlockEditContext?

Do you mean pass down as in by the developer in their own rendering of the component? Technically it's possible because we do pass the UID as a prop to edit, but there was a bit of quality-of-life magic meant to be provided by the InnerBlocks. The component also lives inside editor so it doesn't seem like it should be unadvisable to rely on withBlockEditContext ?

This comment has been minimized.

@gziolo

gziolo Jun 8, 2018

Member

The component also lives inside editor so it doesn't seem like it should be unadvisable to rely on withBlockEditContext ?

Right, I saw a file from core-blocks updated and assumed this one belongs there, too. My bad, please skip my comment :)

@aduth

This comment has been minimized.

Member

aduth commented Jun 19, 2018

Rebased after the merge of #7199. Now no longer need to expose withBlockEditContext from the @wordpress/editor module.

</div>
);
class InnerBlocks extends Component {
componentWillReceiveProps( nextProps ) {

This comment has been minimized.

@youknowriad

youknowriad Jun 20, 2018

Contributor

Can we update this to componentDidUpdate at the same time?

This comment has been minimized.

@aduth

aduth Jun 20, 2018

Member

Can we update this to componentDidUpdate at the same time?

I went down this path and started to realize there's a fair bit about the behavior of block list settings that needs fixing / improvement. Will make a follow up pull request immediately after this merge.

@youknowriad

Nice refactoring

@aduth aduth merged commit 6ba1458 into master Jun 20, 2018

2 checks passed

codecov/project 46.67% (+0.04%) compared to 555f2ea
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the remove/create-inner-block-list branch Jun 20, 2018

@aduth aduth restored the remove/create-inner-block-list branch Jun 20, 2018

@aduth aduth deleted the remove/create-inner-block-list branch Jun 20, 2018

@mtias mtias added this to the 3.1 milestone Jun 21, 2018

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