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.

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

Block Editor: Don't memoize callbacks in 'BlockSettingsDropdown' #59397

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Block Editor: Don't memoize callbacks in 'BlockSettingsDropdown' #59397

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
785f135
Block Editor: Don't memoize callbacks in 'BlockSettingsDropdown'
Mamaduka Feb 27, 2024
01f02a0
Don't pass argument to 'updateSelectionAfterRemove'
Mamaduka Feb 27, 2024
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
84 changes: 38 additions & 46 deletions packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { DropdownMenu, MenuGroup, MenuItem } from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';
import { moreVertical } from '@wordpress/icons';
import { Children, cloneElement, useCallback } from '@wordpress/element';
import { Children, cloneElement } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import {
store as keyboardShortcutsStore,
Expand Down Expand Up @@ -127,41 +127,35 @@ export function BlockSettingsDropdown( {
const isMatch = __unstableUseShortcutEventMatch();
const hasSelectedBlocks = selectedBlockClientIds.length > 0;

const updateSelectionAfterDuplicate = useCallback(
async ( clientIdsPromise ) => {
if ( __experimentalSelectBlock ) {
const ids = await clientIdsPromise;
if ( ids && ids[ 0 ] ) {
__experimentalSelectBlock( ids[ 0 ], false );
}
}
},
[ __experimentalSelectBlock ]
);
async function updateSelectionAfterDuplicate( clientIdsPromise ) {
Copy link
Member

Choose a reason for hiding this comment

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

Only used inline; good to be un-memoized 👍

if ( ! __experimentalSelectBlock ) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reducing the nesting level here 👍

return;
}

const updateSelectionAfterRemove = useCallback( () => {
if ( __experimentalSelectBlock ) {
let blockToFocus = previousBlockClientId || firstParentClientId;
const ids = await clientIdsPromise;
if ( ids && ids[ 0 ] ) {
__experimentalSelectBlock( ids[ 0 ], false );
}
}

// Focus the first block if there's no previous block nor parent block.
if ( ! blockToFocus ) {
blockToFocus = getBlockOrder()[ 0 ];
}
function updateSelectionAfterRemove() {
Copy link
Member

Choose a reason for hiding this comment

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

Only used inline; good to be un-memoized 👍

if ( ! __experimentalSelectBlock ) {
return;
}

// Only update the selection if the original selection is removed.
const shouldUpdateSelection =
hasSelectedBlocks && getSelectedBlockClientIds().length === 0;
let blockToFocus = previousBlockClientId || firstParentClientId;

__experimentalSelectBlock( blockToFocus, shouldUpdateSelection );
// Focus the first block if there's no previous block nor parent block.
if ( ! blockToFocus ) {
blockToFocus = getBlockOrder()[ 0 ];
}
}, [
__experimentalSelectBlock,
previousBlockClientId,
firstParentClientId,
getBlockOrder,
hasSelectedBlocks,
getSelectedBlockClientIds,
] );

// Only update the selection if the original selection is removed.
const shouldUpdateSelection =
hasSelectedBlocks && getSelectedBlockClientIds().length === 0;

__experimentalSelectBlock( blockToFocus, shouldUpdateSelection );
}

// This can occur when the selected block (the parent)
// displays child blocks within a List View.
Expand All @@ -179,20 +173,17 @@ export function BlockSettingsDropdown( {
? undefined
: openedBlockSettingsMenu === currentClientId || false;

const onToggle = useCallback(
( localOpen ) => {
if ( localOpen && openedBlockSettingsMenu !== currentClientId ) {
setOpenedBlockSettingsMenu( currentClientId );
} else if (
! localOpen &&
openedBlockSettingsMenu &&
openedBlockSettingsMenu === currentClientId
) {
setOpenedBlockSettingsMenu( undefined );
}
},
[ currentClientId, openedBlockSettingsMenu, setOpenedBlockSettingsMenu ]
);
function onToggle( localOpen ) {
Copy link
Member

Choose a reason for hiding this comment

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

While this is passed down, it's passed down to useControlledValue():

gutenberg/packages/components/src/dropdown/index.tsx

Line 69 in 01f02a0

onChange: onToggle,

which is then used for the initial value of useState(), thus is utilized only on the first useState() call anyway.

Good to be un-memoized as well 👍

if ( localOpen && openedBlockSettingsMenu !== currentClientId ) {
setOpenedBlockSettingsMenu( currentClientId );
} else if (
! localOpen &&
openedBlockSettingsMenu &&
openedBlockSettingsMenu === currentClientId
) {
setOpenedBlockSettingsMenu( undefined );
}
}

return (
<BlockActions
Expand Down Expand Up @@ -233,7 +224,8 @@ export function BlockSettingsDropdown( {
canRemove
) {
event.preventDefault();
updateSelectionAfterRemove( onRemove() );
onRemove();
updateSelectionAfterRemove();
Copy link
Member Author

Choose a reason for hiding this comment

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

See https://github.com/WordPress/gutenberg/pull/50422/files#r1228033014

} else if (
isMatch(
'core/block-editor/duplicate',
Expand Down
Loading