Avoid using getSelectedBlock and getBlock as much as we can #11811
Conversation
| const firstRemovedBlockClientId = action.clientIds[ 0 ]; | ||
| const state = store.getState(); | ||
| const currentSelectedBlock = getSelectedBlock( state ); | ||
| const currentSelectedBlock = getSelectedBlockClientId( state ); |
There was a problem hiding this comment.
I feel this is a bug fix somehow because we're comparing the previous result with a clientId and not a block object.
There was a problem hiding this comment.
I feel this is a bug fix somehow because we're comparing the previous result with a clientId and not a block object.
I agree. It's probably not made any difference only because selectBlock won't do anything if the ID hasn't changed. Maybe it's useless code? Can be good to avoid dispatching if we don't need it.
Perhaps pull out to a separate pull request?
| * because we want the user to focus on the unregistered block warning, not block settings. | ||
| */ | ||
| if ( ! selectedBlock || isSelectedBlockUnregistered ) { | ||
| if ( ! blockType || ! selectedBlockClientId || isSelectedBlockUnregistered ) { |
There was a problem hiding this comment.
Not sure why the blockType test was necessary here for e2e tests
| return ( props ) => { | ||
| const { align } = props.block.attributes; | ||
| const validAlignments = getBlockValidAlignments( props.block.name ); | ||
| const { align } = props.blockAttributes; |
There was a problem hiding this comment.
Based on build failures, I think the tests for this need to be updated.
| isEmptyDefaultBlock: block && isUnmodifiedDefaultBlock( block ), | ||
| isPreviousBlockADefaultEmptyBlock: previousBlock && isUnmodifiedDefaultBlock( previousBlock ), | ||
| isEmptyDefaultBlock: blockName && isUnmodifiedDefaultBlock( { name: blockName, attributes: blockAttributes } ), | ||
| isPreviousBlockADefaultEmptyBlock: previousBlockClientId && isUnmodifiedDefaultBlock( { |
There was a problem hiding this comment.
We should never want to construct the object of a block. It seems we could use createBlock?
There was a problem hiding this comment.
This is not really returned as a prop though, we just return a boolean.
There was a problem hiding this comment.
I'm not concerned so much about what we expose to the component as a prop, just limiting the exposure to any code which makes an assumption about the block abstraction, should that abstraction come to change in the future.
There was a problem hiding this comment.
yes, I don't like isUnmodifiedDefaultBlock either, I think I'd be fine with createBlock for now.
|
I'm going to split this PR into smaller ones. |
|
Closing as this was exploded into smaller PRs. |
While investigating #11782 I discovered that the
getBlockselector is not very performant even memoized. This PR tries to tweak our components to avoid relying too much on the block objects and use the block Ids and discrete properties instead.I honestly don't know if it did improve the slowness feeling or not. It's very hard to measure properly.