Skip to content
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

GridVisualizer: Avoid over-selecting by using a new getBlockStyles private selector #64386

Merged
merged 2 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 12 additions & 5 deletions packages/block-editor/src/components/grid/grid-visualizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { range, GridRect, getGridInfo } from './utils';
import { store as blockEditorStore } from '../../store';
import { useGetNumberOfBlocksBeforeCell } from './use-get-number-of-blocks-before-cell';
import ButtonBlockAppender from '../button-block-appender';
import { unlock } from '../../lock-unlock';

export function GridVisualizer( { clientId, contentRef, parentLayout } ) {
const isDistractionFree = useSelect(
Expand Down Expand Up @@ -118,19 +119,25 @@ const GridVisualizerGrid = forwardRef(
function ManualGridVisualizer( { gridClientId, gridInfo } ) {
const [ highlightedRect, setHighlightedRect ] = useState( null );

const gridItems = useSelect(
( select ) => select( blockEditorStore ).getBlocks( gridClientId ),
const gridItemStyles = useSelect(
( select ) => {
const { getBlockOrder, getBlockStyles } = unlock(
select( blockEditorStore )
);
const blockOrder = getBlockOrder( gridClientId );
return getBlockStyles( blockOrder );
},
[ gridClientId ]
);
const occupiedRects = useMemo( () => {
const rects = [];
for ( const block of gridItems ) {
for ( const style of Object.values( gridItemStyles ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that it means occupiedRects may no longer be in block order (as it's sourced from an object).

I think it's ok though, the only thing I can see it's used for is this code, which doesn't depend on the ordering:

const isCellOccupied = occupiedRects.some( ( rect ) =>
	rect.contains( column, row )
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah order isn't important.

const {
columnStart,
rowStart,
columnSpan = 1,
rowSpan = 1,
} = block.attributes.style?.layout || {};
} = style?.layout ?? {};
if ( ! columnStart || ! rowStart ) {
continue;
}
Expand All @@ -144,7 +151,7 @@ function ManualGridVisualizer( { gridClientId, gridInfo } ) {
);
}
return rects;
}, [ gridItems ] );
}, [ gridItemStyles ] );

return range( 1, gridInfo.numRows ).map( ( row ) =>
range( 1, gridInfo.numColumns ).map( ( column ) => {
Expand Down
21 changes: 21 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,3 +515,24 @@ export function getTemporarilyEditingFocusModeToRevert( state ) {
export function getInserterSearchInputRef( state ) {
return state.inserterSearchInputRef;
}

/**
* Returns the style attributes of multiple blocks.
*
* @param {Object} state Global application state.
* @param {string[]} clientIds An array of block client IDs.
*
* @return {Object} An object where keys are client IDs and values are the corresponding block styles or undefined.
*/
export const getBlockStyles = createSelector(
( state, clientIds ) =>
clientIds.reduce( ( styles, clientId ) => {
styles[ clientId ] = state.blocks.attributes.get( clientId )?.style;
return styles;
}, {} ),
( state, clientIds ) => [
...clientIds.map(
( clientId ) => state.blocks.attributes.get( clientId )?.style
),
]
);
89 changes: 89 additions & 0 deletions packages/block-editor/src/store/test/private-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getEnabledBlockParents,
getExpandedBlock,
isDragging,
getBlockStyles,
} from '../private-selectors';
import { getBlockEditingMode } from '../selectors';

Expand Down Expand Up @@ -509,4 +510,92 @@ describe( 'private selectors', () => {
);
} );
} );

describe( 'getBlockStyles', () => {
it( 'should return an empty object when no client IDs are provided', () => {
const state = {
blocks: {
attributes: new Map(),
},
};
const result = getBlockStyles( state, [] );
expect( result ).toEqual( {} );
} );

it( 'should return styles for a single block', () => {
const state = {
blocks: {
attributes: new Map( [
[ 'block-1', { style: { color: 'red' } } ],
] ),
},
};
const result = getBlockStyles( state, [ 'block-1' ] );
expect( result ).toEqual( {
'block-1': { color: 'red' },
} );
} );

it( 'should return styles for multiple blocks', () => {
const state = {
blocks: {
attributes: new Map( [
[ 'block-1', { style: { color: 'red' } } ],
[ 'block-2', { style: { fontSize: '16px' } } ],
[ 'block-3', { style: { margin: '10px' } } ],
] ),
},
};
const result = getBlockStyles( state, [
'block-1',
'block-2',
'block-3',
] );
expect( result ).toEqual( {
'block-1': { color: 'red' },
'block-2': { fontSize: '16px' },
'block-3': { margin: '10px' },
} );
} );

it( 'should return undefined for blocks without styles', () => {
const state = {
blocks: {
attributes: new Map( [
[ 'block-1', { style: { color: 'red' } } ],
[ 'block-2', {} ],
[ 'block-3', { style: { margin: '10px' } } ],
] ),
},
};
const result = getBlockStyles( state, [
'block-1',
'block-2',
'block-3',
] );
expect( result ).toEqual( {
'block-1': { color: 'red' },
'block-2': undefined,
'block-3': { margin: '10px' },
} );
} );

it( 'should return undefined for non-existent blocks', () => {
const state = {
blocks: {
attributes: new Map( [
[ 'block-1', { style: { color: 'red' } } ],
] ),
},
};
const result = getBlockStyles( state, [
'block-1',
'non-existent-block',
] );
expect( result ).toEqual( {
'block-1': { color: 'red' },
'non-existent-block': undefined,
} );
} );
} );
} );
Loading