Inserter: Replace the selected block if empty by youknowriad · Pull Request #4917 · WordPress/gutenberg

youknowriad · 2018-02-07T10:23:32Z

this PR update the inserter to replace the selected block if it's an empty block.

Testing instructions

Add an empty paragraph block
Select it
Add an image block from the inserter
The image block should replace the empty block instead of adding a new block

youknowriad · 2018-02-07T10:52:44Z

So I managed to avoid the isEmpty block API by checking that the attributes of the block are equal to a newly inserted default block.

If we detect that the selected block is a default block and its attributes are untouched, replace it
If not, inserter after it

There's a small difference here compared to the original isEmpty implementation because this means if you trigger drop cap or change a color without inserting any text, this block won't be replaced but I guess it makes sense. cc @jasmussen

jasmussen · 2018-02-07T11:07:09Z

Impressive, and thank you for working on this.

When it works, it is glorious.

But I can't get it to work all the time, though:

youknowriad · 2018-02-07T12:45:11Z

@jasmussen What about now?

youknowriad · 2018-02-07T12:58:30Z

Sorry there's an error on the last commit fixing

youknowriad · 2018-02-07T13:03:50Z

So there are still some issues where sometimes the content attribute is equal to [""] which is different from the default value [] so we're considering this as a modified block but in really they are equivalent.

Not sure how to fix this, maybe we should rollback to the isEmpty API as it's more controlled :(

jasmussen · 2018-02-07T13:09:48Z

This is solid work for me. I love it. It definitely works for me.

Though CC: @mtias regarding isEmpty. We might need it.

ellatrix · 2018-02-07T22:39:21Z

So there are still some issues where sometimes the content attribute is equal to [""] which is different from the default value [] so we're considering this as a modified block but in really they are equivalent.

@youknowriad Maybe we can change this in TinyMCE to trigger a change with [] if the content is empty? Maybe this is also "safer" to catch edge cases with all the different situations the field is considered empty. We are already doing an isEmpty check on every selection change, we just need to call this.props.onChange( [] ) there as well.

aduth

I appreciate the effort to avoid expanding the block API 😄

aduth · 2018-02-08T02:02:09Z

blocks/api/helpers.js

+ *
+ * @return {boolean}       Whether the block is an untouched default block
+ */
+export function isUntouchedDefaultBlock( block ) {


Minor: What do you think of "Unmodified" instead of "Untouched" ? Not sure "touch" is the best way for describing modifications to a bock.

aduth · 2018-02-08T02:04:07Z

blocks/api/helpers.js

+	};
+
+	return (
+		block.name === newDefaultBlock.name &&


Minor (performance): This could be an early return to avoid Object.assign cloning of keys into attributeKeys:

const newDefaultBlock = createBlock( getDefaultBlockName() ); if ( block.name !== newDefaultBlock.name ) { return false; } const attributeKeys = { ...keys( newDefaultBlock.attributes ), ...keys( block.attributes ), }; return every( attributeKeys, ( key ) => ( isEqual( newDefaultBlock.attributes[ key ], block.attributes[ key ] ) ) );

aduth · 2018-02-08T02:04:20Z

blocks/api/helpers.js

+ */
+export function isUntouchedDefaultBlock( block ) {
+	const newDefaultBlock = createBlock( getDefaultBlockName() );
+	const attributeKeys = {


Any reason this ought to be an object, rather than an array?

No just my brain making fun of me

aduth · 2018-02-08T02:05:19Z

blocks/api/helpers.js

+
+	return (
+		block.name === newDefaultBlock.name &&
+		every( attributeKeys, ( key ) => isEqual( newDefaultBlock.attributes[ key ], block.attributes[ key ] ) )


Do we want the deep equality check of isEqual, or is shallow sufficient? (via is-equal-shallow)

(Might complicate @iseulde's suggestion of onChange( [] ), but maybe the empty value should be represented as undefined ? or null?)

I think for this function to work properly across several block types (the default block could change and we don't control the attributes shape), this should be a deep equality to avoid bugs.

Performance wise, this is only called when inserting a block (not repeatedly), so probably acceptable.

aduth · 2018-02-08T02:08:31Z

editor/components/inserter/index.js

-				) );
+				const insertedBlock = createBlock( name, { ...initialAttributes, layout } );
+				if ( selectedBlock && isUntouchedDefaultBlock( selectedBlock ) ) {
+					dispatchProps.replaceBlocks( selectedBlock.uid, insertedBlock );


Random thought: Would this be any less complex if we just allowed the block to be inserted as normal, but had a side-effect to remove the previous untouched default block?

What about this feedback?

Oh sorry about that.

Are you saying I should call another action removeBlock + insertBlocks instead of replaceBlocks: We'd still need the selectedBlock prop to check if it's unmodified or not, so we'd still need mergeProps. I'm thinking this doesn't change anything in terms of complexity

Or maybe you're saying we should use effects? Maybe it just adds unnecessary indirection don't you think? It will have the advantage of being more generic (though I'm not sure we want this to be generic: inserting at a given position, no matter how we triggered this insert will trigger the behavior)

Or maybe you're saying we should use effects?

This is more what I had in mind, that upon inserting a new block, if the block preceding it (a) was previously the last block, (b) is of the default block type, and (c) is empty, then it should be removed.

I guess it could be perceived as indirection. I'd framed it in my mind more as a side effect obviously 😄

aduth · 2018-02-08T02:10:53Z

blocks/api/helpers.js

@@ -0,0 +1,32 @@
+/**


Function dumping grounds 😩

As to not be purely cynical: Elsewhere we use "utils" naming? Ought we use that here?

Also, some small tweaks to the isUnmodifiedDefaultBlock function

youknowriad · 2018-02-08T09:39:24Z

Update this per @iseulde proposition and it seems to work consistently now. Let me know if you find another edge case.

youknowriad · 2018-02-08T12:10:38Z

Feedback addressed, merging

Inserter: Replace the selected block if empty

dc25617

youknowriad self-assigned this Feb 7, 2018

youknowriad requested review from aduth and jasmussen February 7, 2018 10:23

youknowriad force-pushed the add/replace-empty-blocks branch from 228466a to 73c4975 Compare February 7, 2018 12:44

Add isUntouchedDefaultBlock helper to avoid the isEmpty API

5581715

youknowriad force-pushed the add/replace-empty-blocks branch from 73c4975 to 5581715 Compare February 7, 2018 13:01

aduth reviewed Feb 8, 2018

View reviewed changes

Rename the isUnmodifiedDefaultBlock function and the helpers file

fba8389

Also, some small tweaks to the isUnmodifiedDefaultBlock function

youknowriad force-pushed the add/replace-empty-blocks branch from 815a5ef to fba8389 Compare February 8, 2018 09:33

RichText: Set an empty array as value if the rich text is empty

dadba9d

youknowriad merged commit 5a6e4ee into master Feb 8, 2018

youknowriad deleted the add/replace-empty-blocks branch February 8, 2018 12:11

aduth mentioned this pull request Feb 16, 2018

Inserter: Use effect for unmodified default block replacement #5090

Closed

Conversation

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

Uh oh!

Uh oh!

youknowriad commented Feb 7, 2018

Uh oh!

jasmussen commented Feb 7, 2018

Uh oh!

youknowriad commented Feb 7, 2018

Uh oh!

youknowriad commented Feb 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

jasmussen commented Feb 7, 2018

Uh oh!

ellatrix commented Feb 7, 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!

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!

youknowriad commented Feb 8, 2018

Uh oh!

youknowriad commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

youknowriad commented Feb 7, 2018 •

edited

Loading

youknowriad commented Feb 7, 2018 •

edited

Loading