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

Replace OffCanvasEditor in browse mode with the List View component #50287

Merged
merged 1 commit into from
May 9, 2023
Merged

Replace OffCanvasEditor in browse mode with the List View component #50287

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
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/list-view/block-select-button.js
View file
Original file line number Diff line number Diff line change
@@ -84,7 +84,11 @@ function ListViewBlockSelectButton(
aria-expanded={ isExpanded }
>
<ListViewExpander onClick={ onToggleExpanded } />
<BlockIcon icon={ blockInformation?.icon } showColors />
<BlockIcon
icon={ blockInformation?.icon }
showColors
context="list-view"
/>
<HStack
alignment="center"
className="block-editor-list-view-block-select-button__label-wrapper"
15 changes: 11 additions & 4 deletions packages/block-editor/src/components/list-view/index.js
View file
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ export const BLOCK_LIST_ITEM_HEIGHT = 36;
* @param {?ComponentType} props.blockSettingsMenu Optional more menu substitution. Defaults to the standard `BlockSettingsDropdown` component.
* @param {string} props.rootClientId The client id of the root block from which we determine the blocks to show in the list.
* @param {string} props.description Optional accessible description for the tree grid component.
* @param {?Function} props.onSelect Optional callback to be invoked when a block is selected. Receives the block object that was selected.
* @param {Function} props.renderAdditionalBlockUI Function that renders additional block content UI.
* @param {Ref} ref Forwarded ref
*/
@@ -78,6 +79,7 @@ function ListViewComponent(
blockSettingsMenu: BlockSettingsMenu = BlockSettingsDropdown,
rootClientId,
description,
onSelect,
renderAdditionalBlockUI,
},
ref
@@ -97,6 +99,7 @@ function ListViewComponent(
const { clientIdsTree, draggedClientIds, selectedClientIds } =
useListViewClientIds( { blocks, rootClientId } );

const { getBlock } = useSelect( blockEditorStore );
const { visibleBlockCount, shouldShowInnerBlocks } = useSelect(
( select ) => {
const {
@@ -130,11 +133,14 @@ function ListViewComponent(
setExpandedState,
} );
const selectEditorBlock = useCallback(
( event, clientId ) => {
updateBlockSelection( event, clientId );
setSelectedTreeId( clientId );
( event, blockClientId ) => {
updateBlockSelection( event, blockClientId );
setSelectedTreeId( blockClientId );
if ( onSelect ) {
onSelect( getBlock( blockClientId ) );
}
},
[ setSelectedTreeId, updateBlockSelection ]
[ setSelectedTreeId, updateBlockSelection, onSelect, getBlock ]
);
useEffect( () => {
isMounted.current = true;
@@ -268,6 +274,7 @@ export default forwardRef( ( props, ref ) => {
showAppender={ false }
blockSettingsMenu={ BlockSettingsDropdown }
rootClientId={ null }
onSelect={ null }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this new prop have a null default instead of having to put it in all list view instances?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to be null at all, or can it just be the default of undefined? Or is it better to flag it as null so that it's explicit that it's not a required prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for setting it as null here is that it prevents people from using it in the ListView component, thus keeping the new props private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's a reset to null after { ...props } I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not a bit confusing that when using the PrivateListView named export the onSelect prop will work as expected, but when using the default export onSelect won't work? (Same thing for the other props that get overridden in the default export).

Should we at least document the default export making it clear that showAppender, blockSettingsMenu, rootClientId and onSelect are not meant to be passed to that component?

renderAdditionalBlockUICallback={ null }
/>
);
6 changes: 3 additions & 3 deletions ...site/src/components/sidebar-navigation-screen-navigation-menus/navigation-menu-content.js
View file
Original file line number Diff line number Diff line change
@@ -144,7 +144,7 @@ export default function NavigationMenuContent( { rootClientId, onSelect } ) {
};
}, [ shouldKeepLoading, clientIdsTree, isLoading ] );

const { OffCanvasEditor, LeafMoreMenu } = unlock( blockEditorPrivateApis );
const { PrivateListView, LeafMoreMenu } = unlock( blockEditorPrivateApis );

const offCanvasOnselect = useCallback(
( block ) => {
@@ -181,14 +181,14 @@ export default function NavigationMenuContent( { rootClientId, onSelect } ) {
<>
{ isLoading && <NavigationMenuLoader /> }
{ ! isLoading && (
<OffCanvasEditor
<PrivateListView
blocks={
isSinglePageList
? clientIdsTree[ 0 ].innerBlocks
: clientIdsTree
}
onSelect={ offCanvasOnselect }
LeafMoreMenu={ LeafMoreMenu }
blockSettingsMenu={ LeafMoreMenu }
showAppender={ false }
renderAdditionalBlockUI={ renderAdditionalBlockUICallback }
/>