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

List View: Expand state if a block is dragged to within a collapsed block in the editor canvas #56493

Merged
merged 1 commit into from
Nov 27, 2023
Merged

List View: Expand state if a block is dragged to within a collapsed block in the editor canvas #56493

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 7 additions & 8 deletions packages/block-editor/src/components/list-view/use-list-view-expand-selected-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,6 @@ export default function useListViewExpandSelectedItem( {
[ firstSelectedBlockClientId ]
);

const parentClientIds =
Array.isArray( selectedBlockParentClientIds ) &&
selectedBlockParentClientIds.length
? selectedBlockParentClientIds
: null;

Comment on lines -30 to -35
Copy link
Contributor Author

@andrewserong andrewserong Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This has been removed so that we can use selectedBlockParentClientIds in the useEffect dependency array — in theory at least, selectedBlockParentClientIds should only be a new array when there really has been a change and we wish to perform an update, since it gets returned by the useSelect instead of being instantiated on each render.

Although, now that I say that, parentClientIds was probably already a reference to that array, so maybe this change wasn't needed 🤔

In any case, I think it's a little neater without parentClientIds, but happy to revert if folks prefer.

Copy link
Member

Choose a reason for hiding this comment

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

If selectedBlockParentClientIds?.length does the same job, LGTM to remove it :)

// Expand tree when a block is selected.
useEffect( () => {
// If the selectedTreeId is the same as the selected block,
Expand All @@ -42,15 +36,20 @@ export default function useListViewExpandSelectedItem( {
}

// If the selected block has parents, get the top-level parent.
if ( parentClientIds ) {
if ( selectedBlockParentClientIds?.length ) {
// If the selected block has parents,
// expand the tree branch.
setExpandedState( {
type: 'expand',
clientIds: selectedBlockParentClientIds,
} );
}
}, [ firstSelectedBlockClientId ] );
}, [
firstSelectedBlockClientId,
selectedBlockParentClientIds,
selectedTreeId,
setExpandedState,
] );

return {
setSelectedTreeId,
Expand Down