Add block list change detection #7325

Luehrsen · 2018-06-16T12:13:58Z

Description

This PR is a result of issue #5626 to be able to react to the deletion of blocks. The added actions can be used to hook into the process of adding or deleting blocks to react accordingly.

This is a fixed PR of #7310.

How has this been tested?

Tested on local machine. Unit tests should probably be written, but I would need assistance with that.

Types of changes

Added a set of new actions for the post editor to hook into the post removal/addition/transformation process.

Checklist:

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

tofumatt

As you mentioned I think this could use unit tests but I also think there's a fair amount of code cleanup required here. I've left some comments asking for refactoring and clarification.

tofumatt · 2018-06-17T09:30:18Z

edit-post/hooks/block-list-change-detection/index.js

+/**
+ * A compare helper for lodash's difference by
+ *
+ * @param {Component} block A block object.


Nitpick, but I think these should line up (see the docblock below, it looks nicer 😄)

tofumatt · 2018-06-17T09:32:20Z

edit-post/hooks/block-list-change-detection/index.js

+import { subscribe, select } from '@wordpress/data';
+
+/**
+ * A compare helper for lodash's difference by


I would write differenceBy function instead of difference by... as-is it looks like a half-finished sentence so caught me off-guard.

For clarity and grammar maybe:

/** * A comparison helper for use with lodash's differenceBy function [...]

tofumatt · 2018-06-17T09:35:49Z

edit-post/hooks/block-list-change-detection/index.js

+ * not just additions/removals. Therefore we actually compare the array with a
+ * previous state and look for changes in length or uid.
+ *
+ * @param {function} selector Selector.


I think this parameter could be expanded; what is an example of an argument you'd pass here?

tofumatt · 2018-06-17T09:35:53Z

edit-post/hooks/block-list-change-detection/index.js

+ * previous state and look for changes in length or uid.
+ *
+ * @param {function} selector Selector.
+ * @param {function} listener Listener.


I think this parameter could be expanded; what is an example of an argument you'd pass here?

tofumatt · 2018-06-17T09:36:35Z

edit-post/hooks/block-list-change-detection/index.js

+ // For performance reasons we first check the cheap length change,
+ // before we actuallly do a deep object comparison
+ if ( selectedBlocks.length !== previousBlocks.length ) {
+ // The block list length has changed, so there is obviously a change event happening


I don't think this comment is needed if the above is refactored.

tofumatt · 2018-06-17T10:08:29Z

edit-post/hooks/block-list-change-detection/index.js

+ if ( selectedBlocks.length !== previousBlocks.length ) {
+ // The block list length has changed, so there is obviously a change event happening
+ listener( selectedBlocks, previousBlocks );
+ previousBlocks = selectedBlocks;


Both of these variables are assigned to selector()... why would they change?

One is the old state (previousBlock) and the new state (selectedBlocks). Once a block in the editor is added the block list is getting updated and the selectedBlocks object changes.

tofumatt · 2018-06-17T10:09:18Z

edit-post/hooks/block-list-change-detection/index.js

+/**
+ * Subscribe to block data
+ *
+ * This function subscribes to block data, compares old and new states upon


What block data is this subscribing to? I'm sorry but I'm finding the comments here a bit vague.

tofumatt · 2018-06-17T10:10:24Z

edit-post/hooks/block-list-change-detection/index.js

+ * This function subscribes to block data, compares old and new states upon
+ * change and fires actions accordingly.
+ */
+subscribe( onBlocksChangeListener( select( 'core/editor' ).getBlocks, ( blocks, oldBlocks, difference = null ) => {


I find difference to be a vague variable name, could you clarify?

Should i add it to the documentation or change the variable name? Technically it is an intersection between blocks & oldBlocks, verbally it is the difference between these two arrays. (To signify that there is a difference, even though the length is equal.)

What kind of value would it have? I don't understand what it means to be "the difference between these two arrays"... does that mean it's all the elements in either array not shared or just a boolean that signifies "yes these arrays are different"?

Docs around it would be great but if it's a boolean that is true when the arrays are different and false when not, something like blocksHaveBeenChanged or something might better? 🤷‍♂️

Ah, I see the point. Currently difference is an array of blocks that have been added. That signals that when the length of the blocklist stays equal, but there is a block in difference, that there was a transformation event.

I have changed the var to a boolean value hasChanged to signal that.

tofumatt · 2018-06-17T10:20:30Z

edit-post/hooks/block-list-change-detection/index.js

+ // When the length is equal, but a change hapened, we have a transformation
+ if ( oldBlocks.length === blocks.length && difference ) {
+ // A block has been deleted
+ for ( const i in deletedBlocks ) {


Could we do:

deletedBlocks.each( ( block ) => { const actionName = `blocks.transformed.from.${ camelCase( block.name ) }`; doAction( actionName, block ); } )

instead?

tofumatt · 2018-06-17T14:37:33Z

edit-post/hooks/block-list-change-detection/index.js

+ }
+
+ // A block has been added
+ for ( const i in addedBlocks ) {


It seems like this logic of looping through blocks and calling doAction is common; you could probably make a function that takes the list of blocks and the prefix rather than duplicating the action each time (eg it would look like doActionForBlocks( 'blocks.transformed.from', deletedBlocks ))

gziolo · 2018-06-18T13:02:41Z

Can you provide an example how this implementation could help to solve the original issue raised in #5626?

aduth · 2018-06-18T13:33:34Z

Noted concern of trying to address this problem from the client in #5626 (comment) .

Luehrsen · 2018-06-18T13:51:37Z

Can you provide an example how this implementation could help to solve the original issue raised in #5626?

Given a hypothetical block named 'gb/opener', that saves it's state in a meta key called opener, the following code could be used to remove the meta upon removal of the block.

/**
 * An action listener, which fires the deletion of the metadata
 * once the remove action is seen.
 */
addAction('blocks.removed.gbOpener', 'gbOpener/removeMeta', ( block ) => {
	let currentPostMeta = select( 'core/editor' ).getEditedPostAttribute( 'meta' );
	currentPostMeta.opener = '';
	dispatch( 'core/editor' ).editPost( { meta: currentPostMeta } );
});

Other usecases would be to notify a plugin about the presence of a certain block. Or do some things on a block transformation. Or trigger some NUX, when a block is being added for the first time by this user.

youknowriad · 2018-12-28T09:06:28Z

After thinking about this, I think if we do this API, we open a pandora box to provide such APIs for a plethora of events. I'm leaning towards keeping this plugin territory for now.

gziolo · 2019-01-31T17:58:26Z

I tend to agree with @youknowriad. @aduth what's your take on this since you commented on the parent issue.

aduth · 2019-01-31T19:01:51Z

Per my comment on #5626, if this were addressed from within core (handling the deletion of a meta associated with a removed block) during save, regardless where the save came from (since this problem exists for e.g. anything interfacing with the REST API to save post content), then the primary need for this pull request goes away anyways.

gziolo · 2019-02-01T08:02:23Z

I see what you mean. Inded, it makes a lot of sense. Even if we would land this change, this issue would still be present if someone uses REST API to remove one of the blocks. Given that mobile team is working on Gutenberg integration for the mobile app this issue will become eve more prominent.

On the technical side I see that this PR is using getBlocks selector which was identified as one of the less performant ones.

subscribe( onBlocksChangeListener( select( 'core/editor' ).getBlocks...

It might be hard to implement the feature proposed which is performant enough given what we learned in the recent months. I'm inclined to close this PR and seek for PHP based solution. I'm wondering if the root issue shouldn't be moved to core now as the logic for saving blocks is there.

@Luehrsen many thanks for all the time you spend working on this PR. It helped a lot to better understand what is required to fix this issue.

Luehrsen added 4 commits March 22, 2018 15:53

Merge remote-tracking branch 'WordPress/master'

334098e

Merge remote-tracking branch 'upstream/master'

9484512

Merge branch 'master' of https://github.com/WordPress/gutenberg

6770de2

Rebased new branch

fc4fc48

tofumatt reviewed Jun 17, 2018

View reviewed changes

Code Cleanup & Documentation

521ac1f

gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Jun 18, 2018

gziolo added the [Feature] Blocks Overall functionality of blocks label Jun 18, 2018

Luehrsen added 3 commits June 18, 2018 16:31

Clarify variable name

d09ce27

Added README.md

2785a69

Fix Readme

8f7ad51

gziolo closed this Feb 1, 2019

gziolo mentioned this pull request Feb 1, 2019

Add hook for block deletion so post meta can be removed #5626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block list change detection #7325

Add block list change detection #7325

Luehrsen commented Jun 16, 2018 •

edited by tofumatt

Loading

tofumatt left a comment

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

Luehrsen Jun 18, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

Luehrsen Jun 18, 2018

tofumatt Jun 18, 2018

Luehrsen Jun 18, 2018

tofumatt Jun 17, 2018

tofumatt Jun 17, 2018

gziolo commented Jun 18, 2018

aduth commented Jun 18, 2018

Luehrsen commented Jun 18, 2018

youknowriad commented Dec 28, 2018

gziolo commented Jan 31, 2019

aduth commented Jan 31, 2019

gziolo commented Feb 1, 2019

Add block list change detection #7325

Add block list change detection #7325

Conversation

Luehrsen commented Jun 16, 2018 • edited by tofumatt Loading

Description

How has this been tested?

Types of changes

Checklist:

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jun 18, 2018

aduth commented Jun 18, 2018

Luehrsen commented Jun 18, 2018

youknowriad commented Dec 28, 2018

gziolo commented Jan 31, 2019

aduth commented Jan 31, 2019

gziolo commented Feb 1, 2019

Luehrsen commented Jun 16, 2018 •

edited by tofumatt

Loading