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: Simplify the BlockNavigation component #31290

Merged
merged 7 commits into from
May 21, 2021
Merged

List View: Simplify the BlockNavigation component #31290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b9e59f3
Simplify the BlockNavigation component
Copons Apr 28, 2021
6238fe0
Minor fixes
Copons Apr 30, 2021
6f96374
Restructure useBlockNavigationClientIds hook for readability
Copons Apr 30, 2021
06a0d01
Move noop to const
Copons May 11, 2021
91acff1
Simplify post editor list view
Copons May 17, 2021
ef1b5fa
Remove the BlockNavigation component as it's not currently used anymore
Copons May 17, 2021
757f20b
Remove unnecessary selectedClientIds prop in hooks
Copons May 20, 2021
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
16 changes: 12 additions & 4 deletions packages/block-editor/src/components/block-navigation/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { listView } from '@wordpress/icons';
/**
* Internal dependencies
*/
import BlockNavigation from './';
import BlockNavigationTree from './tree';
import { store as blockEditorStore } from '../../store';

function BlockNavigationDropdownToggle( {
Expand Down Expand Up @@ -60,9 +60,17 @@ function BlockNavigationDropdown(
/>
) }
renderContent={ () => (
<BlockNavigation
__experimentalFeatures={ __experimentalFeatures }
/>
<div className="block-editor-block-navigation__container">
<p className="block-editor-block-navigation__label">
{ __( 'List view' ) }
</p>

<BlockNavigationTree
showNestedBlocks
showOnlyCurrentHierarchy
__experimentalFeatures={ __experimentalFeatures }
/>
</div>
) }
/>
);
Expand Down
81 changes: 0 additions & 81 deletions packages/block-editor/src/components/block-navigation/index.js
View file
Open in desktop

This file was deleted.

42 changes: 37 additions & 5 deletions packages/block-editor/src/components/block-navigation/tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,56 @@
*/

import { __experimentalTreeGrid as TreeGrid } from '@wordpress/components';
import { useEffect, useMemo, useRef } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';
import { useCallback, useEffect, useMemo, useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import BlockNavigationBranch from './branch';
import { BlockNavigationContext } from './context';
import useBlockNavigationClientIds from './use-block-navigation-client-ids';
import useBlockNavigationDropZone from './use-block-navigation-drop-zone';
import { store as blockEditorStore } from '../../store';

const noop = () => {};

/**
* Wrap `BlockNavigationRows` with `TreeGrid`. BlockNavigationRows is a
* recursive component (it renders itself), so this ensures TreeGrid is only
* present at the very top of the navigation grid.
*
* @param {Object} props Components props.
* @param {boolean} props.__experimentalFeatures Flag to enable experimental features.
* @param {boolean} props.__experimentalPersistentListViewFeatures Flag to enable features for the Persistent List View experiment.
* @param {Object} props Components props.
* @param {Array} props.blocks Custom subset of block client IDs to be used instead of the default hierarchy.
* @param {Function} props.onSelect Block selection callback.
* @param {boolean} props.showNestedBlocks Flag to enable displaying nested blocks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expose/handle this property explicitly and set it as default to true? Currently we have four usages of this component where all pass this prop with true.

Copy link
Member

Choose a reason for hiding this comment

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

Even though we almost have this enabled everywhere, personally I'd still prefer to opt-in rather than opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion either way, to be honest.
I haven't paid much attention to that prop; it has been there for quite a while now, and I have no idea why it was introduced instead of just defaulting it to true. 🤔

* @param {boolean} props.showOnlyCurrentHierarchy Flag to limit the list to the current hierarchy of blocks.
* @param {boolean} props.__experimentalFeatures Flag to enable experimental features.
* @param {boolean} props.__experimentalPersistentListViewFeatures Flag to enable features for the Persistent List View experiment.
*/
export default function BlockNavigationTree( {
blocks,
showOnlyCurrentHierarchy,
onSelect = noop,
__experimentalFeatures,
__experimentalPersistentListViewFeatures,
...props
} ) {
const { clientIdsTree, selectedClientIds } = useBlockNavigationClientIds(
blocks,
showOnlyCurrentHierarchy,
__experimentalPersistentListViewFeatures
);
const { selectBlock } = useDispatch( blockEditorStore );
const selectEditorBlock = useCallback(
( clientId ) => {
selectBlock( clientId );
onSelect( clientId );
},
[ selectBlock, onSelect ]
);

let {
ref: treeGridRef,
target: blockDropTarget,
Expand Down Expand Up @@ -62,7 +89,12 @@ export default function BlockNavigationTree( {
ref={ treeGridRef }
>
<BlockNavigationContext.Provider value={ contextValue }>
<BlockNavigationBranch { ...props } />
<BlockNavigationBranch
blocks={ clientIdsTree }
selectBlock={ selectEditorBlock }
selectedBlockClientIds={ selectedClientIds }
{ ...props }
/>
</BlockNavigationContext.Provider>
</TreeGrid>
);
Expand Down
88 changes: 88 additions & 0 deletions packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* WordPress dependencies
*/

import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { isClientIdSelected } from './utils';
import { store as blockEditorStore } from '../../store';

const useBlockNavigationSelectedClientIds = (
__experimentalPersistentListViewFeatures
) =>
useSelect(
( select ) => {
const {
getSelectedBlockClientId,
getSelectedBlockClientIds,
} = select( blockEditorStore );

if ( __experimentalPersistentListViewFeatures ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make any difference somewhere? getSelectedBlockClientIds will first check if it's single block selected and return it in an Array.

Copy link
Member

Choose a reason for hiding this comment

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

This is a confusing function 😬 I'd expect it to return an Array in all cases. Yet we return a single string on line 32.

We should just use getSelectedBlockClientIds() directly. We can also remove all the Array.isArray checks and just check for length property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well what do you know, I think you're right! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or actually...

The problem here is that some consumers (Dropdown to be specific) really expect the selected client ID to be single and string, or null otherwise, whereas the Persistent List View wants the whole multiselection array.

For example, getBlockHierarchyRootClientId (used here and defined here) will use the parameter straight away as object property.

Using those two distinct functions basically saves a bunch of additional checks and conversions to make sure everything keeps receiving the parameters with the expected types.

This said, I think the major cause of confusion here is this:

const isSingleBlockSelected =
	selectedClientIds && ! Array.isArray( selectedClientIds );

Single selections can be arrays as well, and the naming here doesn't make it clear.
I guess isNotMultiselectedBlock would not make it much clearer, so I'm more oriented to just adding a comment to add some context.

return getSelectedBlockClientIds();
}

return getSelectedBlockClientId();
},
[ __experimentalPersistentListViewFeatures ]
);

const useBlockNavigationClientIdsTree = (
blocks,
selectedClientIds,
showOnlyCurrentHierarchy
) =>
useSelect(
( select ) => {
const {
getBlockHierarchyRootClientId,
__unstableGetClientIdsTree,
__unstableGetClientIdWithClientIdsTree,
} = select( blockEditorStore );

if ( blocks ) {
return blocks;
}

const isSingleBlockSelected =
selectedClientIds && ! Array.isArray( selectedClientIds );
if ( ! showOnlyCurrentHierarchy || ! isSingleBlockSelected ) {
return __unstableGetClientIdsTree();
}

const rootBlock = __unstableGetClientIdWithClientIdsTree(
getBlockHierarchyRootClientId( selectedClientIds )
);
if ( ! rootBlock ) {
return __unstableGetClientIdsTree();
}
Addison-Stavlo marked this conversation as resolved.
Show resolved Hide resolved

const hasHierarchy =
! isClientIdSelected( rootBlock.clientId, selectedClientIds ) ||
( rootBlock.innerBlocks && rootBlock.innerBlocks.length !== 0 );
if ( hasHierarchy ) {
return [ rootBlock ];
}

return __unstableGetClientIdsTree();
},
[ blocks, selectedClientIds, showOnlyCurrentHierarchy ]
);

export default function useBlockNavigationClientIds(
blocks,
showOnlyCurrentHierarchy,
__experimentalPersistentListViewFeatures
) {
const selectedClientIds = useBlockNavigationSelectedClientIds(
__experimentalPersistentListViewFeatures
);
const clientIdsTree = useBlockNavigationClientIdsTree(
blocks,
selectedClientIds,
showOnlyCurrentHierarchy
);
return { clientIdsTree, selectedClientIds };
}
25 changes: 6 additions & 19 deletions packages/block-library/src/navigation/block-navigation-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,38 +5,25 @@ import {
__experimentalBlockNavigationTree,
store as blockEditorStore,
} from '@wordpress/block-editor';
import { useSelect, useDispatch } from '@wordpress/data';
import { useSelect } from '@wordpress/data';

export default function BlockNavigationList( {
clientId,
__experimentalFeatures,
} ) {
const { blocks, selectedBlockClientId } = useSelect(
( select ) => {
const {
getSelectedBlockClientId,
__unstableGetClientIdsTree,
} = select( blockEditorStore );

return {
blocks: __unstableGetClientIdsTree( clientId ),
selectedBlockClientId: getSelectedBlockClientId(),
};
},
const blocks = useSelect(
( select ) =>
select( blockEditorStore ).__unstableGetClientIdsTree( clientId ),
[ clientId ]
);

const { selectBlock } = useDispatch( blockEditorStore );

return (
<__experimentalBlockNavigationTree
Addison-Stavlo marked this conversation as resolved.
Show resolved Hide resolved
blocks={ blocks }
selectedBlockClientIds={ [ selectedBlockClientId ] }
selectBlock={ selectBlock }
__experimentalFeatures={ __experimentalFeatures }
showNestedBlocks
showAppender
showBlockMovers
showNestedBlocks
__experimentalFeatures={ __experimentalFeatures }
/>
);
}
16 changes: 2 additions & 14 deletions packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
useInstanceId,
useMergeRefs,
} from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';
import { useDispatch } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { ESCAPE } from '@wordpress/keycodes';
Expand All @@ -23,16 +23,6 @@ import { ESCAPE } from '@wordpress/keycodes';
import { store as editPostStore } from '../../store';

export default function ListViewSidebar() {
const { clientIdsTree, selectedBlockClientIds } = useSelect( ( select ) => {
const {
__unstableGetClientIdsTree,
getSelectedBlockClientIds,
} = select( blockEditorStore );
return {
clientIdsTree: __unstableGetClientIdsTree(),
selectedBlockClientIds: getSelectedBlockClientIds(),
};
}, [] );
const { setIsListViewOpened } = useDispatch( editPostStore );

const { clearSelectedBlock, selectBlock } = useDispatch( blockEditorStore );
Expand Down Expand Up @@ -73,9 +63,7 @@ export default function ListViewSidebar() {
ref={ useMergeRefs( [ focusReturnRef, focusOnMountRef ] ) }
>
<BlockNavigationTree
blocks={ clientIdsTree }
selectBlock={ selectEditorBlock }
selectedBlockClientIds={ selectedBlockClientIds }
onSelect={ selectEditorBlock }
showNestedBlocks
__experimentalPersistentListViewFeatures
/>
Expand Down
16 changes: 2 additions & 14 deletions packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
useInstanceId,
useMergeRefs,
} from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';
import { useDispatch } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { ESCAPE } from '@wordpress/keycodes';
Expand All @@ -23,16 +23,6 @@ import { ESCAPE } from '@wordpress/keycodes';
import { store as editSiteStore } from '../../store';

export default function ListViewSidebar() {
const { clientIdsTree, selectedBlockClientIds } = useSelect( ( select ) => {
const {
__unstableGetClientIdsTree,
getSelectedBlockClientIds,
} = select( blockEditorStore );
return {
clientIdsTree: __unstableGetClientIdsTree(),
selectedBlockClientIds: getSelectedBlockClientIds(),
};
}, [] );
const { setIsListViewOpened } = useDispatch( editSiteStore );

const { clearSelectedBlock, selectBlock } = useDispatch( blockEditorStore );
Expand Down Expand Up @@ -73,9 +63,7 @@ export default function ListViewSidebar() {
ref={ useMergeRefs( [ focusReturnRef, focusOnMountRef ] ) }
>
<BlockNavigationTree
blocks={ clientIdsTree }
selectBlock={ selectEditorBlock }
selectedBlockClientIds={ selectedBlockClientIds }
onSelect={ selectEditorBlock }
showNestedBlocks
__experimentalPersistentListViewFeatures
/>
Expand Down