Block List: Extract copy, scroll-into-view logic to separate non-visual components

[aduth]

This pull request seeks to refactor copy and scroll-into-view logic currently contained within BlockListLayout to two separate top-level non-visual components CopyHandler and MultiSelectScrollIntoView. The goal is to optimize rendering of BlockListLayout, which currently re-renders when any blocks change because of its dependency on getMultiSelectedBlocks. Further, since CopyHandler and MultiSelectScrollIntoView is neither specific to a list layout rendering nor has any visual effect, it is well-suited as a non-visual component.

Testing instructions:

Verify that there are no regressions in the copying, cutting, or scroll-into-view assurance of multi-selection and multi-selection expansions.

[youknowriad]

It should be easier to use a prop for this className now since it's an "external" className.

[aduth]

I'm wondering if this is necessary at all...

[youknowriad]

Any reason for using query here instead of connect

[aduth]

Any reason for using query here instead of connect

I guess it speaks to a broader question: Do we want to start discouraging use of connect altogether? My thinking is that it speaks strongly to its viability if we're forced to use it in first-party code. Also a nice abstraction to avoid interacting with Redux directly. Also avoids some problems with variable shadowing and importing selectors.

[mcsf]

Also avoids some problems with variable shadowing and importing selectors.

Can you elaborate a little? Or are these separate things, i.e. you mean variable shadowing for actions (see our recent discussion)?

[aduth]

Can you elaborate a little?

I'll elaborate in the form of a refactoring pull request demonstrating the benefits 😆

See: #5083

[youknowriad]

Why are we exposing those selectors?

[aduth]

Ah, I think at https://github.com/WordPress/gutenberg/pull/5021/files#diff-a6cb47a97748d7742a26ae134e49e8a1R76 I started down the path of using query but realized I have no access to dispatch which is necessary 😄

[aduth]

In latest rebase:

• Removes registration of unused selectors
• Uses new selector API signature
• Replaced reference to editor scrollable layout with reused getScrollContainer originally in BlockListBlock, moved to editor DOM utilities file

[aduth]

In a subsequent pull request, I could imagine creating a separate component for keeping the currently selected block in view. Does this approach for non-visual components seem sensible to you @youknowriad ? I like that because they return null; from render, the reconciliation is very simple even if the props are changing.

[youknowriad]

@aduth Actually, I like these components a lot, I wonder though if they should be included by the layout module "edit-post" or implicitly inside BlockList or something.

Also, I was wondering about the MultiSelectScrollIntoView component and whether it makes sense to use it to handle the scrolling behavior when moving block as well. It would make a global EditorScrollManager or something

[youknowriad]

In general, it looks like step by step, we're moving all this imperative code relying on DOM nodes to components like this (including WritingFlow in the mix). It kind of feel like the right thing to do to isolate these behaviors.

[aduth]

Yeah, my only concern so far with this is that it moves access of the DOM node from being ref-bound to querySelector. Wondering if we ought to at least isolate these as well (some getBlockDOMNode helper).