Refactor selectors used in copy-handler #11844
Conversation
|
|
||
| // We only care about this value when the copy is performed | ||
| // We call it dynamically in the event handler to avoid unnecessary re-renders. | ||
| getBlocks, |
There was a problem hiding this comment.
As far as I remember withDispatch wraps those handlers with proxies to ensure they don’t update on every prop change. So you can’t execute select outside of those handlers as they will become immediately stale. We will have to document it properly to avoid any bugs. It is advanced usage which is necessary mostly in core, so it might be okey to attack it this way.
There was a problem hiding this comment.
I will open PR and let's discuss it there.
| @@ -26,18 +26,18 @@ class CopyHandler extends Component { | |||
| } | |||
|
|
|||
| onCopy( event ) { | |||
| const { multiSelectedBlocks, selectedBlock } = this.props; | |||
| const { selectedBlockClientIds, getBlocks } = this.props; | |||
There was a problem hiding this comment.
| const { selectedBlockClientIds, getBlocks } = this.props; | |
| const { selectedBlockClientIds = [], getBlocks } = this.props; |
would it be possible to use to simplify next conditional check?
There was a problem hiding this comment.
Actually it's an array in all cases, I'll just remove the first check.
| @@ -46,12 +46,12 @@ class CopyHandler extends Component { | |||
| } | |||
|
|
|||
| onCut( event ) { | |||
| const { multiSelectedBlockClientIds } = this.props; | |||
| const { selectedBlockClientIds } = this.props; | |||
There was a problem hiding this comment.
The same as above with defaulting to an empty array.
|
|
||
| this.onCopy( event ); | ||
|
|
||
| if ( multiSelectedBlockClientIds.length ) { | ||
| this.props.onRemove( multiSelectedBlockClientIds ); | ||
| if ( selectedBlockClientIds && selectedBlockClientIds.length ) { |
There was a problem hiding this comment.
const selectedBlockClientIds = selectedBlockClientId ? [ selectedBlockClientId ] : getMultiSelectedBlockClientIds();Should there be a check whethere there is more than one id in here?
There was a problem hiding this comment.
Actually, I'd add a boolean flag hasMultiSelection it's more secure
youknowriad
force-pushed
the
refactor/copy-handler
branch
from
b8dde83
to
875d911
Compare
Extracted form #11811
In this PR, I avoid using the not-so performant selectors
getSelectedBlockandgetMultiSelectedBlocksin thewithSelectHoC and I replace them withgetSelectedBlockClientIdand agetBlocksselectors only used in the block handler.