Skip to content

DRY up ContentBlocksList and BlockInspectorLockedBlocks #51281

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.

Sign up for GitHub

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

Merged
noisysocks merged 6 commits into from
Jun 9, 2023
Merged

DRY up ContentBlocksList and BlockInspectorLockedBlocks #51281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e4569f5
DRY up ContentBlocksList and BlockInspectorLockedBlocks
noisysocks Jun 7, 2023
1f46387
Add back check that ensures list items don't appear in list
noisysocks Jun 7, 2023
d154a8d
Remove unnecessary CSS
noisysocks Jun 7, 2023
06f29ee
Merge remote-tracking branch 'origin/trunk' into update/dry-up-conten…
noisysocks Jun 8, 2023
6006434
Reduce amount of rerendering
noisysocks Jun 8, 2023
e4421e8
Update __experimentalGetGlobalBlocksByName
noisysocks Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 18 additions & 92 deletions packages/block-editor/src/components/block-inspector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,8 @@ import {
hasBlockSupport,
store as blocksStore,
} from '@wordpress/blocks';
import {
FlexItem,
PanelBody,
__experimentalHStack as HStack,
__experimentalVStack as VStack,
Button,
__unstableMotion as motion,
} from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useMemo, useCallback } from '@wordpress/element';
import { PanelBody, __unstableMotion as motion } from '@wordpress/components';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand All @@ -28,7 +20,6 @@ import MultiSelectionInspector from '../multi-selection-inspector';
import BlockVariationTransforms from '../block-variation-transforms';
import useBlockDisplayInformation from '../use-block-display-information';
import { store as blockEditorStore } from '../../store';
import BlockIcon from '../block-icon';
import BlockStyles from '../block-styles';
import DefaultStylePicker from '../default-style-picker';
import { default as InspectorControls } from '../inspector-controls';
Expand All @@ -38,77 +29,26 @@ import AdvancedControls from '../inspector-controls-tabs/advanced-controls-panel
import PositionControls from '../inspector-controls-tabs/position-controls-panel';
import useBlockInspectorAnimationSettings from './useBlockInspectorAnimationSettings';
import BlockInfo from '../block-info-slot-fill';

function useContentBlocks( blockTypes, block ) {
const contentBlocksObjectAux = useMemo( () => {
return blockTypes.reduce( ( result, blockType ) => {
if (
blockType.name !== 'core/list-item' &&
Object.entries( blockType.attributes ).some(
( [ , { __experimentalRole } ] ) =>
__experimentalRole === 'content'
)
) {
result[ blockType.name ] = true;
}
return result;
}, {} );
}, [ blockTypes ] );
const isContentBlock = useCallback(
( blockName ) => {
return !! contentBlocksObjectAux[ blockName ];
},
[ contentBlocksObjectAux ]
);
return useMemo( () => {
return getContentBlocks( [ block ], isContentBlock );
}, [ block, isContentBlock ] );
}

function getContentBlocks( blocks, isContentBlock ) {
const result = [];
for ( const block of blocks ) {
if ( isContentBlock( block.name ) ) {
result.push( block );
}
result.push( ...getContentBlocks( block.innerBlocks, isContentBlock ) );
}
return result;
}

function BlockNavigationButton( { blockTypes, block, selectedBlock } ) {
const { selectBlock } = useDispatch( blockEditorStore );
const blockType = blockTypes.find( ( { name } ) => name === block.name );
const isSelected =
selectedBlock && selectedBlock.clientId === block.clientId;
return (
<Button
isPressed={ isSelected }
onClick={ () => selectBlock( block.clientId ) }
>
<HStack justify="flex-start">
<BlockIcon icon={ blockType.icon } />
<FlexItem>{ blockType.title }</FlexItem>
</HStack>
</Button>
);
}
import BlockQuickNavigation from '../block-quick-navigation';
import { unlock } from '../../lock-unlock';

function BlockInspectorLockedBlocks( { topLevelLockedBlock } ) {
const { blockTypes, block, selectedBlock } = useSelect(
const contentClientIds = useSelect(
( select ) => {
return {
blockTypes: select( blocksStore ).getBlockTypes(),
block: select( blockEditorStore ).getBlock(
topLevelLockedBlock
),
selectedBlock: select( blockEditorStore ).getSelectedBlock(),
};
const {
getClientIdsOfDescendants,
getBlockName,
getBlockEditingMode,
} = unlock( select( blockEditorStore ) );
return getClientIdsOfDescendants( [ topLevelLockedBlock ] ).filter(
Copy link
Member

@Mamaduka Mamaduka Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice trick to get around the array/object referential equality "requirement" and avoid excessive re-renders 🥇

Copy link
Member Author

@noisysocks noisysocks Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... it is? Won't filter return a new array every time? 😅

Copy link
Member Author

@noisysocks noisysocks Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sent me down a rabbit hole of optimising all of this. I made some progress but will tackle it in a separate PR 😅

Copy link
Member

@Mamaduka Mamaduka Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that 😅 I left two more comments in other PR, where returned values need optimization.

This map select returns one dimensional array of client IDs, correct?

Copy link
Member Author

@noisysocks noisysocks Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment? #50643 (comment) Yes I've been looking into that.

Yes this is a one dimensional array.

Copy link
Member Author

@noisysocks noisysocks Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 6006434 which reduces the number of re-renders in PagePanels and BlockQuickNavigation by selecting the minimal amount of data. I think BlockInspector here is as good as we can get it without creating a new selector which is not worth it right now imo.

I have more performance improvements ready but they're unrelated so they'll be in a separate PR 😀

Copy link
Member Author

@noisysocks noisysocks Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: #51319

( clientId ) =>
getBlockName( clientId ) !== 'core/list-item' &&
getBlockEditingMode( clientId ) === 'contentOnly'
);
},
[ topLevelLockedBlock ]
);
const blockInformation = useBlockDisplayInformation( topLevelLockedBlock );
const contentBlocks = useContentBlocks( blockTypes, block );
return (
<div className="block-editor-block-inspector">
<BlockCard
Expand All @@ -117,23 +57,9 @@ function BlockInspectorLockedBlocks( { topLevelLockedBlock } ) {
/>
<BlockVariationTransforms blockClientId={ topLevelLockedBlock } />
<BlockInfo.Slot />
<VStack
spacing={ 1 }
padding={ 4 }
className="block-editor-block-inspector__block-buttons-container"
>
<h2 className="block-editor-block-card__title">
{ __( 'Content' ) }
</h2>
{ contentBlocks.map( ( contentBlock ) => (
<BlockNavigationButton
selectedBlock={ selectedBlock }
key={ contentBlock.clientId }
block={ contentBlock }
blockTypes={ blockTypes }
/>
) ) }
</VStack>
<PanelBody title={ __( 'Content' ) }>
<BlockQuickNavigation clientIds={ contentClientIds } />
</PanelBody>
</div>
);
}
Expand Down
5 changes: 0 additions & 5 deletions packages/block-editor/src/components/block-inspector/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,3 @@
.block-editor-block-inspector__tab-item {
flex: 1 1 0px;
}

.block-editor-block-inspector__block-buttons-container {
border-top: $border-width solid $gray-200;
padding: $grid-unit-20;
}
74 changes: 74 additions & 0 deletions packages/block-editor/src/components/block-quick-navigation/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import {
Button,
__experimentalVStack as VStack,
__experimentalHStack as HStack,
FlexItem,
} from '@wordpress/components';
import { getBlockType, __experimentalGetBlockLabel } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import BlockIcon from '../block-icon';

export default function BlockQuickNavigation( { clientIds } ) {
Copy link
Member Author

@noisysocks noisysocks Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not certain about what to name this component.

if ( ! clientIds.length ) {
return null;
}
return (
<VStack spacing={ 1 }>
{ clientIds.map( ( clientId ) => (
<BlockQuickNavigationItem
key={ clientId }
clientId={ clientId }
/>
) ) }
</VStack>
);
}

function BlockQuickNavigationItem( { clientId } ) {
const { name, attributes, isSelected } = useSelect(
( select ) => {
const {
getBlockName,
getBlockAttributes,
isBlockSelected,
hasSelectedInnerBlock,
} = select( blockEditorStore );
return {
name: getBlockName( clientId ),
attributes: getBlockAttributes( clientId ),
isSelected:
isBlockSelected( clientId ) ||
hasSelectedInnerBlock( clientId, /* deep: */ true ),
};
},
[ clientId ]
);
const { selectBlock } = useDispatch( blockEditorStore );
const blockType = getBlockType( name );
return (
<Button
key={ clientId }
isPressed={ isSelected }
onClick={ () => selectBlock( clientId ) }
>
<HStack justify="flex-start">
<BlockIcon icon={ blockType.icon } />
<FlexItem>
{ __experimentalGetBlockLabel(
blockType,
attributes,
'list-view'
) }
</FlexItem>
</HStack>
</Button>
);
}
2 changes: 2 additions & 0 deletions packages/block-editor/src/private-apis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import BlockInfo from './components/block-info-slot-fill';
import { useShouldContextualToolbarShow } from './utils/use-should-contextual-toolbar-show';
import { cleanEmptyObject } from './hooks/utils';
import { useBlockEditingMode } from './components/block-editing-mode';
import BlockQuickNavigation from './components/block-quick-navigation';

/**
* Private @wordpress/block-editor APIs.
Expand All @@ -26,4 +27,5 @@ lock( privateApis, {
useShouldContextualToolbarShow,
cleanEmptyObject,
useBlockEditingMode,
BlockQuickNavigation,
} );
5 changes: 4 additions & 1 deletion packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,13 @@ export const __experimentalGetGlobalBlocksByName = createSelector(
if ( ! blockName ) {
return EMPTY_ARRAY;
}
const blockNames = Array.isArray( blockName )
? blockName
: [ blockName ];
const clientIds = getClientIdsWithDescendants( state );
const foundBlocks = clientIds.filter( ( clientId ) => {
const block = state.blocks.byClientId.get( clientId );
return block.name === blockName;
return blockNames.includes( block.name );
Comment on lines +306 to +312
Copy link
Member

@Mamaduka Mamaduka Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noisysocks, do you remember why you introduced this change? I can't find any place that passes the block names array to this selector.

} );
return foundBlocks.length > 0 ? foundBlocks : EMPTY_ARRAY;
},
Expand Down
77 changes: 0 additions & 77 deletions packages/edit-site/src/components/sidebar-edit-mode/page-panels/content-blocks-list.js
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions packages/edit-site/src/components/sidebar-edit-mode/page-panels/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { useMemo } from '@wordpress/element';
import { store as editSiteStore } from '../../../store';
import useEditedEntityRecord from '../../use-edited-entity-record';
import SidebarCard from '../sidebar-card';
import ContentBlocksList from './content-blocks-list';
import PageContent from './page-content';

export default function PagePanels() {
const context = useSelect(
Expand Down Expand Up @@ -62,7 +62,7 @@ export default function PagePanels() {
/>
</PanelBody>
<PanelBody title={ __( 'Content' ) }>
<ContentBlocksList />
<PageContent />
</PanelBody>
<PanelBody title={ __( 'Template' ) }>
<VStack>
Expand Down
27 changes: 27 additions & 0 deletions packages/edit-site/src/components/sidebar-edit-mode/page-panels/page-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import {
store as blockEditorStore,
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import { PAGE_CONTENT_BLOCK_TYPES } from '../../page-content-focus/constants';
import { unlock } from '../../../private-apis';

const { BlockQuickNavigation } = unlock( blockEditorPrivateApis );

export default function PageContent() {
const clientIds = useSelect(
( select ) =>
select( blockEditorStore ).__experimentalGetGlobalBlocksByName(
PAGE_CONTENT_BLOCK_TYPES
),
[]
);
return <BlockQuickNavigation clientIds={ clientIds } />;
}