Block Editor: Simplify Selection Reducer

[ellatrix]

Description

Splitting out some stuff from #16428.

I've moved isSelectionEnabled, isMultiSelecting, and initialPosition out of the blockSelection, as these are not tied to the selection state. These can all be independent reducers, which is easier to maintain. Since the selection reducer didn't contain anything but the start and end of the selection, I flattened the structure to state.selectionStart and state.selectionEnd. The reducers ended up being much simpler.

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[mcsf]

Overall this looks like a step in the right direction, but I left a couple of questions.

[mcsf]

This reducer is a little unconventional, in that its default behaviour isn't to return state, but rather to implicitly return undefined. Is this the best way to proceed? Seems like it could increase the likelihood of mistakes in the future.

[ellatrix]

How so? The reducer should store the initial position temporarily, until another action occurs. The only exception is REMOVE_BLOCKS. As mentioned above in the doc, the reducer should eventually be removed in favour of setting selection directly.

[mcsf]

This is more or less the same observation as in #17482 (comment)

I'll defer to your judgment, as I don't want to block this.

[ellatrix]

Ok, I'll add a comment.

[mcsf]

These flattened reducers have some overlap in the types of actions to which they react. One thing that worries me then is that, by only testing them as independent reducers, we may be omitting useful tests that look at the compound state of these reducers.

Could we keep this state compound while still benefitting from the simplicity of flattened reducers? What if, in reducers, we still write selectionStart et al. as independent functions, but at the end combine them as blockSelection: combineReducers( { selectionStart, selectionEnd, initialPosition, isMultiSelecting } )? Or is this just completely against the objectives behind this PR?

[ellatrix]

What's the overlap? selection and selectionStart/selectionEnd don't have any overlap. isMultiSelecting doesn't have any overlap with the others either. initialPosition has a very limited overlap, where the state is very different from the rest. I don't really see a problem here.

[mcsf]

If you're confident about test coverage as reducers become decoupled, I trust you!

[ellatrix]

Thanks @mcsf! I adjusted the tests so both the final selectionStart and selectionEnd are tested. I also added a comment for the unconventional reducer.