Remove inserter inside multi-selection

[ellatrix]

Description

Fixes #3076.

How Has This Been Tested?

Make a multi block selection. Make sure there are inserters around the block (visible by hover), but not inside the block.

Checklist:

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

[ellatrix]

🙈 More elegant solutions welcome.

[codecov]

Codecov Report

Merging #3246 into master will increase coverage by <.01%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##           master    #3246      +/-   ##
==========================================
+ Coverage   34.65%   34.65%   +<.01%     
==========================================
  Files         260      260              
  Lines        6741     6752      +11     
  Branches     1220     1225       +5     
==========================================
+ Hits         2336     2340       +4     
- Misses       3719     3723       +4     
- Partials      686      689       +3

Impacted Files	Coverage Δ
editor/modes/visual-editor/sibling-inserter.js	0% <0%> (ø)	⬆️
editor/modes/visual-editor/block-list.js	0% <0%> (ø)	⬆️
editor/selectors.js	93.12% <80%> (-0.36%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f31557...62875b0. Read the comment docs.

[aduth]

Elsewhere I've been combatting frequent re-renders of this component where the common culprit is the multi-selection props (see #3247, #3170). I think we would be better served, both for intent and performance, to have more of a separation between multi-selection and block list rendering.

As it impacts here, I wonder if it should be the responsibility of the sibling inserter to determine if it should be visible (or return null) depending on if the insertIndex is within range of the current multi-selection?

[ellatrix]

@aduth Works for me too. Should we then pass down the multi selection props, or should it get all that itself?

[ellatrix]

(Because the props are already present here.)

[aduth]

It should get itself, since this will trigger a render only on the individual component when the prop changes.

[aduth]

(Because the props are already present here.)

This is the issue though, since a change in reference value on multiSelectedBlocks and multiSelectedBlockUids causes a re-render on the full block listing.

[ellatrix]

@aduth I'm not sure how we'd avoid that though. Do you have any ideas?

[aduth]

In this case I would suggest passing as a boolean prop into <VisualEditorSiblingInserter /> via connect, perhaps the result of a new selector isBlockWithinMultiSelect( state, uid ), and short-cutting the render (return null;) if truthy. Maybe a different selector name if the logic needs to be "within, except the last index".

In a normal case I wouldn't be too bothered one way or the other whether this logic is controlled by the list component or the child itself, but since inserter is otherwise a simple component and the block list component is excessively complex, it could help readability to delegate this behavior to the child.

In this specific circumstance, there's probably not much of a performance difference, only because we already have multiSelectedBlockUids available; the point might be to not have this prop passed in, since without memoization passing transformed array props will trigger a render by the new reference. So in general the advice is:

• Pass simple props
• Memoize if you can't (but this is a crutch)
• It can be easier if you move to the connect logic of the child component, e.g. the simple boolean passed into VisualEditorSiblingInserter

[mcsf]

Rebased and refactored according to review suggestions. I only now saw the reference to #3485; hopefully the two fixes can fix together nicely.

[aduth]

With uid being passed, can (and/or should) we determine insertIndex in its connect selector?

[mcsf]

can we? should we?

We can. To answer the question of should, I tried doing it in b3218c7. Looks OK enough; insertIndex defaults to 0 to account for the one place where SiblingInsert was used without an associated uid. That default is something you may object to. Let me know.

[aduth]

LGTM 👍

[aduth]

This seems like a strangely unexpected behavior for a function called isBlockWithinSelection. On its name, I'd expect it to return true if the block is anywhere in the current selection, regardless of multi-selection, singular selection, even considering block hierarchy (I arrive here from #15499 (comment) and relevant current logic of block list async rendering). Excluding the last block in the multi-selection is odd, even if for the particular requirements of the UI it's a necessary exclusion.

Do you think it's something which would make sense to revise the behavior and account for the UI needs in some other way, like a separate selector call to test whether the selected clientId is last in a multi-selection?