Inserter: Leverage provisional block deletion for unmodified default

[aduth]

This pull request seeks to simplify the inserter's onInsertBlock implementation to take advantage of existing provisional block deletion (#5417). The intended behavior is preserved: When inserting a block via an inserter and the selected block is an unmodified default block, the act of insertion should trigger the deletion of the default block.

Notably, this avoids the need for selectedBlock to be passed to the Inserter, which is a source of frequent renders for the component.

Implementation notes:

Block selection state tracks from an INSERT_BLOCKS action type, so it was necessary to add INSERT_BLOCKS as the triggering action for the removeProvisionalBlock effect handler. Alternatively, we could consider selection from insert a side-effect of the insert itself, removing handling of insertion from the blockSelection state and leveraging the existing SELECT_BLOCK effect handler for provisional block deletion.

In future iterations, it might be worth exploring whether we can collapse the concepts of "unmodified default block" with "provisional block", as on the surface they appear to be referring to the same type of block.

Testing instructions:

Verify that when inserting a block after having created a new default block (paragraph), the unmodified default block is removed.

The "Adding Blocks" end-to-end test has been revised to test for this behavior:

npm run test-e2e

[noisysocks]

Love this simplification! 😍

It's causing a very small regression, though. If you press ENTER and then insert a block the placeholder block is not replaced.

Before:

After:

[aduth]

Nice catch @noisysocks . There's actually a couple things going on here:

• Our default "Enter" behavior for a block was to manually insert a paragraph block, not to use the insertDefaultBlock action creator, so it wasn't inserted as provisional. This is updated in 8746b66.
• Because of the RichText split behavior, when pressing Enter at the end of a paragraph, we were creating the block as a split block where the second new block had empty content, but wasn't technically the default block (and therefore not provisional). In 28e735f, I changed the behavior so a block being provisional is considered by the reducer, even if it's not explicitly created via insertDefaultBlock.

I made the end-to-end tests more thorough to account for your behavior (and others) in 28e735f.

cc also @youknowriad, since you may have an opinion on explicitness here.

[youknowriad]

It's weird that the <br> is not between "Cite" and the rest. Is this a test issue only or a real issue.

[aduth]

It's weird that the <br> is not between "Cite" and the rest. Is this a test issue only or a real issue.

Hmm I totally missed this. Will look into it.

[youknowriad]

I personally don't know if we want to consider a new block created with "Enter" as provisional. Imagine this use case, you have a bunch of paragraphs and you want to add "space" between them. you can just click Enter at the end of a paragraph.

With this change, the paragraph is removed.

I don't have strong opinion but the code is good.

[aduth]

I personally don't know if we want to consider a new block created with "Enter" as provisional. Imagine this use case, you have a bunch of paragraph and you want to add "space" between them. you can just click Enter at the end of a paragraph.

I'm not sure what the best answer is here. Why should it be considered provisional if the paragraph block is created with the between-inserter, but not if it's created by an Enter press ? Why does the paragraph get replaced if we proceed to use the inserter after the Enter press, if it's not provisional ? For both the technical implementation and the user experience, we'd benefit from consistency here.

Related: Spacer block (#6121) could be used to achieve such an intended goal for intentional spacing.

cc @jasmussen Would you have any thoughts on the expected behavior here?

[jasmussen]

Interesting discussion, I love it. We're entering an aspect of syntactic refinement that indicates polish, this fact alone is great, so awesome work.

I think we're also entering a key aspect of editors, where we have to decide on a direction, choosing between freedom to do random potentially suboptimal things on the one side, and automatically doing "helpful" cleanup work on the other side and potentially going overboard.

Take Medium, for example, they are definitely in the camp of doing too much cleanup work in my opinion. For example place your cursor in the middle of a word and press and hold spacebar. You'll only ever make a single space, every subsequent one will be eaten with no indication what's up.

Similarly I just switched from Atom to VS Code. Atom was always kind to ensure I had a trailing linebreak at the end of code files. VS Code does not do that. This is one act of "helpfulness" that I'm missing.

It's a very delicate balance in other words, and often subject to personal opinion. Having just checked out this branch, this feels right. It feels logical and consistent. Besides, you can create TWO linebreaks and both will be inserted, and there's a spacer block — we could potentially even look at auto-converting two empty paragraphs to a spacer block.

It can't hurt to hear more opinions on this, CC: @karmatosed @mtias

GIF:

[aduth]

Similarly I just switched from Atom to VS Code. Atom was always kind to ensure I had a trailing linebreak at the end of code files. VS Code does not do that. This is one act of "helpfulness" that I'm missing.

Tangent: You probably want to install EditorConfig for VS Code

[jasmussen]

Tangent: You probably want to install EditorConfig for VS Code

Whoooa nice, that looks great. Do you know if it works with Settings Sync?

[aduth]

Whoooa nice, that looks great. Do you know if it works with Settings Sync?

No idea, I don't use VS Code 😄 Merely remarking on the fact that you may have had a similar EditorConfig plugin for Atom, or otherwise that the project includes an .editorconfig which, when a complementing plugin is present, will ensure the trailing newline:

gutenberg/.editorconfig

Line 12 in 87638ce

insert_final_newline = true

[aduth]

This has stagnated from indecision. Going to close, but cherry-pick some less controversial changes into new pull requests.