Fix calculation of new block index on drop #10201
Conversation
oandregal
added
the
[Feature] Drag and Drop
| fromIndex: index, | ||
| rootClientId, | ||
| clientId, | ||
| layout, |
There was a problem hiding this comment.
layout was unnecessary so I took advantage of this PR to clean that as well in c7af125
| rootClientId, | ||
| clientId, | ||
| layout, | ||
| srcIndex: index, |
There was a problem hiding this comment.
Improve nomenclature, as it was inconsistent (fromIndex but not fromClientId?), especially when this data was used in the BlockDropZone.
- if it is drop into one of its dropzones - if it is drop into one of its descendant blocks' dropzones
oandregal
force-pushed
the
fix/drop-move-calculation
branch
from
19a3421 to
b53d8c6
Compare
oandregal
added
the
[Priority] High
oandregal
changed the title
oandregal
changed the title
| // so we also need to account for that case separately. | ||
| return ( srcRoot === dstRoot ) || ( ! srcRoot === true && ! dstRoot === true ); | ||
| }; | ||
| const isSameBlock = ( src, dst ) => src === dst; |
There was a problem hiding this comment.
Minor: I feel extracting this to a separate function doesn't provide anything "useful" as it's just a === check. (nothing block related)
There was a problem hiding this comment.
I know it seems straightforward code, but I like how having the function helps document the intent for the reader.
| } catch ( err ) { | ||
| if ( ! isBlockDropType( type ) || | ||
| isSameBlock( srcClientId, dstClientId ) || | ||
| isSrcBlockAnAncestorOfDstBlock( srcClientId, dstClientId ) ) { |
There was a problem hiding this comment.
Minor: Could it be useful to extract the behavior of this function to something like getInsertIndex( src, dest ) to test it properly?
| * | ||
| * @return {Array} ids of descendants. | ||
| */ | ||
| export const getDescendants = ( state, clientIds ) => flatMap( clientIds, ( clientId ) => { |
There was a problem hiding this comment.
I feel the name is not very explicit. Could it be getBlocksDescendantsClientIds or something in that vein?
There was a problem hiding this comment.
Renamed it to getClientIdsOfDescendants in bebfdbd.
Fixes #9823, #10202 and #9303.
Testing