Blocks: Support reusable nested blocks (reusable blocks refactor) by aduth · Pull Request #5228 · WordPress/gutenberg

aduth · 2018-02-23T21:27:00Z

Supersedes #5010
Cherry-picks 840fbec and ee61e18 from #5010
Cherry-picks dde1787 and 80e03de from #5223
Cherry-picks 66fdda5 from #5220

This pull request seeks to support reusable nested blocks. Unlike #5010, the changes here do not expand the reusable blocks API to support nested blocks, but rather refactor reusable blocks to serve as pointers to the original block references, therefore having better natural support for nested blocks.

A few of the changes have been brought along from #5010:

Move context assignment of inner block list from BlockListBlock to BlockEdit, as the latter is the component used in rendering from core/block's edit
- Downside is a circular dependency from blocks to editor, as BlockEdit requires access to the BlockList component
Update reusable block UI behaviors to not abort editing when the block becomes deselected, as this is incompatible with nested blocks where selection changes to the inner block

A complication of reusable blocks as pointers it that we need to add support for the blocks state to receive arbitrary blocks after initial load without dirtying state (the loaded result of a reusable block should be merged into but not dirtying to blocks state). New options of ignoreTypes have been added to both the withChangeDetection and withHistory higher-order reducers to accommodate this.

Testing instructions:

Verify there are no regressions in the behavior of reusable blocks.

Ensure that you can save a Columns block, including content, as a reusable block.

Ensure unit tests pass:

npm test

Remaining tasks:

Reusable blocks effects tests

aduth · 2018-03-07T02:47:51Z

Rebased to resolve conflicts. Also made an update in 5c6cd14 which avoids referencing the wp.blocks.BlockList global and respects contextual toolbar and block menu settings.

For the editable controls removed in 7d0bdde, I think there might be a solution involving withFocusOutside, but it's a bit tricky because focus is lost when transitioning into Edit mode because the Edit button disappears, so we need to find some other way to transition focus back into the block after editing has started. We should be able to just dispatch selectBlock on the block and rely on the selection focusTabbable behavior, but this was not working in my testing (on a Columns reusable block).

Still need to reinstate effects tests.

noisysocks

This is looking great. Really nice work 😍

Two issues I noticed in my testing:

Reusable blocks are not displaying at all in the inserter. They display correctly on master. I can see that the FETCH happens, so my guess is that it's a regression in the getInserterItems selector.
If you have a reusable block in the editor and you reload the page, there is a moment where the Block has been deleted or is unavailable message appears for about a single frame, right after the loading spinner hides. My guess is that this message appears in between when FETCH_REUSABLE_BLOCKS_SUCCESS is dispatched and when RECEIVE_REUSABLE_BLOCKS is dispatched.

noisysocks · 2018-02-27T04:48:08Z

editor/store/selectors.js

+		return {
+			...block,
+			id: ref,
+			isTemporary: ! Number.isInteger( ref ),


Should we split this out into a seperate isReusableBlockTemporary selector? This would match isFetchingReusableBlock and isSavingReusableBlock.

noisysocks · 2018-02-27T04:51:19Z

blocks/library/block/index.js

+				{ element }
+				{ ( isSelected || isEditing ) && (
+					<ReusableBlockEditPanel
+						key="panel"


We can omit this now that we're using <Fragment>.

No longer relevant.

noisysocks · 2018-02-27T04:53:27Z

blocks/block-edit/index.js

+
+		// `edit` and `save` are functions or components describing the markup
+		// with which a block is displayed. If `blockType` is valid, assign
+		// them preferencially as the render value for the block.


s/preferencially/preferentially/

Fixed in f4eb391.

noisysocks · 2018-03-07T00:52:20Z

blocks/block-edit/index.js

+	} ) ),
+	withAPIData( ( { postType } ) => ( {
+		user: `/wp/v2/users/me?post_type=${ postType }&context=edit`,
+	} ) ),


I've seen this pattern in a few places now. I wonder if it's worth encapsulating this in a new withUserCapabilities HOC?

Probably not necessary for now.

With #5219, I might imagine this could be a simple selector on a core data module.

noisysocks · 2018-03-07T03:53:04Z

blocks/library/block/index.js

+			isFetchingReusableBlock,
+			isSavingReusableBlock,
+			getBlock,
+		} = select( 'core/editor' );


This is great ❤️

noisysocks · 2018-03-07T04:32:45Z

editor/store/effects.js


 		const { id } = action;
 		const { getState, dispatch } = store;
+		const state = getState();


nit: Could avoid assigning getState to its own variable now that it's only used once.

const { id } = action; const { dispatch } = store; const state = store.getState();

Fixed in 4e1026f.

noisysocks · 2018-03-07T04:46:26Z

editor/store/effects.js

+			} )
+		) );
+
+		dispatch( receiveBlocks( [ parsedBlock ] ) );


This confused me for a minute. Let's add a comment explaining that it's necessary to put parsedBlock back into the store because replaceBlock will have removed it.

Fixed in 08333d3.

noisysocks · 2018-03-07T04:48:15Z

editor/store/effects.js

+		const parsedBlock = getBlock( getState(), action.uid );
+		const reusableBlock = {
+			id: uniqueId( 'reusable' ),
+			uid: parsedBlock.uid,


Hmm, this id vs uid distinction is extra confusing now they're in the same object 😬

I'm more and more convinced we should rename uid to id throughout the codebase. This can then be:

const reusableBlock = { id: uniqueId( 'reusable' ), blockId: parsedBlock.id,

No need to do this right now though.

Couldn't naming the property blockUID achieve the same effect as what you're describing?

noisysocks · 2018-03-07T05:41:30Z

editor/store/test/selectors.js

 	} );

+	describe( 'isFetchingReusableBlock', () => {
+		it( 'should return false when the block is not being saved', () => {


s/saved/fetched/

Fixed in c348a43.

noisysocks · 2018-03-07T05:41:34Z

editor/store/test/selectors.js

+			expect( isFetching ).toBe( false );
+		} );
+
+		it( 'should return true when the block is being saved', () => {


s/saved/fetched/

Fixed in c348a43.

pento · 2018-03-09T06:23:13Z

🎶 Ain't no party like a recursive block party, because a recursive block party don't stop. 🎶

I ran into the same problem with the inserter as @noisysocks, so I did this by manually editing the shared block in the database.

Create a Nested Block
Save it as a Shared Block
Edit the new Shared block manually, to include itself as a column.
Reload Gutenberg
🎉

It'd probably be a good idea to implement recursion limits on this. I'd probably start with not allowing a nested shared block to contain itself, then let's see if there's potential for abuse of nested block trees before we worry too much about putting limits on it.

noisysocks · 2018-03-09T22:52:23Z

I rebased, addressed my own feedback, fixed those two issues that I described, and fixed an additional issue I discovered where deleting a reusable block would cause blocks that were created via 'Convert to Regular Block' to be removed.

Still to come: update the effects tests that were removed.

I addressed all of my own feedback

noisysocks · 2018-03-13T01:10:33Z

This should be good to go. Care to give it a quick check, @aduth?

aduth

I'm noticing that the default block appender no longer shows at all when editing nested blocks. Thinking this could perhaps be related to recent refactoring with it (#5478).

We should probably also wait to merge this until after the next release, since it's large enough to risk breakage.

aduth · 2018-03-14T00:50:45Z

editor/components/block-preview/index.js

 /**
 * Internal dependencies
 */
+import { createInnerBlockList } from '../block-list/utils';


I don't love that we reach into the local utilities of a separate component. Seems like if we need it in both places, might be best to extract up a level to a common place.

👌 moved in e99f333aaeb2626b40f4c03c6fb8d5996443d50c.

aduth · 2018-03-14T00:58:24Z

editor/store/selectors.js

 */
 export function getReusableBlocks( state ) {
-	return Object.values( state.reusableBlocks.data );
+	return Object.keys( state.reusableBlocks.data ).map( ( ref ) => getReusableBlock( state, ref ) );


Minor: Lodash's _.map works well on objects for this purpose (also trivially more performant by avoiding a second iteration):

return map( state.reusableBlocks.data, ( data, ref ) => getReusableBlock( state, ref ) );

👌 changed in 7d87636ec2d95a9e678423fa66390ee2649af4ad.

noisysocks · 2018-03-14T18:04:28Z

I'm noticing that the default block appender no longer shows at all when editing nested blocks.

It looks like this is broken in master. I ran git bisect and it seems that 530a66e is what caused it.

We should probably also wait to merge this until after the next release, since it's large enough to risk breakage.

👍

This reverts commit 8872058.

In nested context, block selection changes from reusable block to the inner block being edited, but we want to keep the UI shown

e.g. within Disabled component, e.g. within reusable nested block

Generate BlockList from block's own utils, which has advantage of: - Not having circular dependency from blocks to editor - Respecting block menu and contextual toolbar props

Makes `getInserterItems` and `getFrecentInserterItems` respect that reusable blocks now point to a block that is elsewhere in the editor state. This makes reusable blocks again appear in the inserter.

Allow reusable nested blocks to be previewed in the inserter by having BlockPreview inject a createInnerBlockList function via context.

Re-order the FETCH_REUSABLE_BLOCKS effect so that the reusable block is added to the store before it is marked as no longer being fetched. This prevents core/block from briefly flashing the 'Not found' error message in between dispatches.

Making a copy of the referenced block prevents the regular block from being removed should the reuasble block be later deleted.

Use _.map to avoid iterating through the reusable blocks twice in `getReusableBlocks`.

aduth · 2018-03-16T13:20:31Z

This is looking good, and I'd like it to have a few days to sit in master ahead of the next release 👍

The regression with columns default appender should be addressed separately, since it was not introduced here.

aduth · 2018-03-16T13:21:08Z

Thanks for your help with this pull request @noisysocks

aduth added [Feature] Blocks Overall functionality of blocks [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Feb 23, 2018

This was referenced Feb 23, 2018

Blocks: Support reusable nested blocks #5010

Closed

Store: Mutate editor store reference in middlewares application #5229

Merged

noisysocks mentioned this pull request Feb 27, 2018

Allow blocks to specify that they can't be made reusable #5280

Closed

noisysocks self-requested a review February 27, 2018 04:37

aduth requested a review from a team February 27, 2018 18:21

aduth modified the milestone: 2.3 Feb 27, 2018

aduth force-pushed the fix/reusable-blocks-pointers branch 2 times, most recently from 8a970d1 to 5c6cd14 Compare March 7, 2018 02:43

noisysocks previously requested changes Mar 7, 2018

View reviewed changes

noisysocks force-pushed the fix/reusable-blocks-pointers branch from 5c6cd14 to e34e523 Compare March 9, 2018 20:31

noisysocks force-pushed the fix/reusable-blocks-pointers branch from a33325c to ba90cbc Compare March 13, 2018 01:04

aduth commented Mar 14, 2018

View reviewed changes

noisysocks force-pushed the fix/reusable-blocks-pointers branch from ba90cbc to e99f333 Compare March 14, 2018 18:04

noisysocks mentioned this pull request Mar 14, 2018

Writing Flow: Redirect click below editor to last text field #5541

Merged

aduth added 7 commits March 14, 2018 11:33

Revert "Blocks: Disable Convert to Reusable for nested blocks"

e53d036

This reverts commit 8872058.

Block: Move block context provides into BlockEdit

e0ea667

Blocks: Avoid dismissing editable controls

8501e7d

In nested context, block selection changes from reusable block to the inner block being edited, but we want to keep the UI shown

State: Enhance history reducer utils to support ignores

edc7681

Block List: Hide disabled default block appender

928c2d1

e.g. within Disabled component, e.g. within reusable nested block

State: Update reusable blocks as pointers to state blocks

7aff115

Block List: Inject inner block list creator as function

f7dabde

Generate BlockList from block's own utils, which has advantage of: - Not having circular dependency from blocks to editor - Respecting block menu and contextual toolbar props

noisysocks added 10 commits March 14, 2018 11:33

Typo: saved -> fetched

d5da664

Typo: preferencially -> preferentially

4aac14a

Explain why dispatching RECEIVE_BLOCKS is necessary

b4afb64

Remove unnecessary getState variable

b2f16d0

Update getInserterItems to respect new reusable block data layout

04bfd26

Makes `getInserterItems` and `getFrecentInserterItems` respect that reusable blocks now point to a block that is elsewhere in the editor state. This makes reusable blocks again appear in the inserter.

Provide createInnerBlockList context in BlockPreview

c169500

Allow reusable nested blocks to be previewed in the inserter by having BlockPreview inject a createInnerBlockList function via context.

When converting reusable -> regular, replace the block with a copy

1b3f1ba

Making a copy of the referenced block prevents the regular block from being removed should the reuasble block be later deleted.

Avoid iterating through reusable blocks twice

4025b7a

Use _.map to avoid iterating through the reusable blocks twice in `getReusableBlocks`.

Move createInnerBlockList into editor/utils

3f0180c

noisysocks force-pushed the fix/reusable-blocks-pointers branch from e99f333 to 3f0180c Compare March 14, 2018 18:33

aduth merged commit f27e228 into master Mar 16, 2018

aduth deleted the fix/reusable-blocks-pointers branch March 16, 2018 13:20

This was referenced Mar 20, 2018

Converting a Columns block into a reusable block fails #4923

Closed

Block API: Drop removed createReusableBlock function #5702

Merged

This was referenced Mar 20, 2018

Pressing Cancel while editing a reusable block does not reset the block preview #5719

Closed

Fix reusable block cancelling #5723

Merged

Editing post in Text Mode causes shared blocks to go away #5754

Closed

noisysocks mentioned this pull request Apr 10, 2018

Add shared blocks to the blocks autocompleter #6067

Merged

3 tasks

aduth mentioned this pull request May 24, 2018

Editor: Simplify block context to eliminate createInnerBlockList #6943

Closed

noisysocks mentioned this pull request Jun 4, 2018

Shared Blocks: Refactor fetching/saving/updating to avoid cycle dependency #7080

Closed

youknowriad mentioned this pull request Jun 4, 2018

Reusable Blocks: Reimplement reusable block as embedded editor #7119

Closed

aduth mentioned this pull request Jun 11, 2018

State: Trigger autosave as standard save for draft by current user #7130

Merged

Conversation

aduth commented Feb 23, 2018 • edited by noisysocks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noisysocks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pento commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noisysocks commented Mar 9, 2018

Uh oh!

noisysocks commented Mar 13, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noisysocks commented Mar 14, 2018

Uh oh!

aduth commented Mar 16, 2018

Uh oh!

aduth commented Mar 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aduth commented Feb 23, 2018 •

edited by noisysocks

Loading

aduth commented Mar 7, 2018 •

edited

Loading

noisysocks left a comment •

edited

Loading

pento commented Mar 9, 2018 •

edited

Loading