New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the block inspector component to avoid getSelectedBlock #11842

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Nov 14, 2018

Extracted from #11811

This PR refactors the block inspector component to use getSelectedBlockClientId and getBlockName instead of getSelectedBlock which is not very performant.

@youknowriad youknowriad added this to the 4.4 milestone Nov 14, 2018

@youknowriad youknowriad self-assigned this Nov 14, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Nov 14, 2018

/*
* If the selected block is of an unregistered type, avoid showing it as an actual selection
* because we want the user to focus on the unregistered block warning, not block settings.
*/
if ( ! selectedBlock || isSelectedBlockUnregistered ) {
if ( ! blockType || ! selectedBlockClientId || isSelectedBlockUnregistered ) {

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

If I don't add blockType check it fails the e2e test, not sure why.

@jorgefilipecosta jorgefilipecosta self-requested a review Nov 14, 2018

@jorgefilipecosta

Things tested well in my smoke tests. The changes seem fine, I left a possible improvement. I would prefer if we had a conclusion on the cause why ! blockType is needed, but the check does not seems ilogical so I dont' think it is a blocker.

@@ -79,15 +78,17 @@ const BlockInspector = ( { selectedBlock, blockType, count, hasBlockStyles } ) =
export default withSelect(
( select ) => {
const { getSelectedBlock, getSelectedBlockCount } = select( 'core/editor' );
const { getSelectedBlockClientId, getSelectedBlockCount, getBlockName } = select( 'core/editor' );
const { getBlockStyles } = select( 'core/blocks' );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 14, 2018

Member

I think we should use getBlockType from select ( 'core/blocks' ); instead of the wp.blocks one, otherwise when there are updates to the blocks our withSelect function may not be executed.
This may be related to the reason end 2 end tests were failing without the blockType check.

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

I agree. I think we should avoid relying on getBlockType (the global) entirely but I didn't want to create inconsistency between components now. This should be done as a dedicated PR at some point.

@youknowriad youknowriad merged commit c6bbfb5 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the refactor/block-inspector branch Nov 15, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment