Block Editor: Simplify Selection Reducer #17467
Conversation
ellatrix
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
ellatrix
force-pushed
the
try/simpler-selection-reducer
branch
from
b99c181 to
5e9803a
Compare
ellatrix
added
[Package] Block editor
| } else if ( action.type === 'REMOVE_BLOCKS' ) { | ||
| return state; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I'll add a comment.
| selectionStart, | ||
| selectionEnd, | ||
| initialPosition, | ||
| isMultiSelecting, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ellatrix
force-pushed
the
try/simpler-selection-reducer
branch
from
5e9803a to
56c281c
Compare
|
Thanks @mcsf! I adjusted the tests so both the final |
Description
Splitting out some stuff from #16428.
I've moved
isSelectionEnabled,isMultiSelecting, andinitialPositionout of theblockSelection, 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 tostate.selectionStartandstate.selectionEnd. The reducers ended up being much simpler.How has this been tested?
Screenshots
Types of changes
Checklist: