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

Off Canvas Navigation Editor: Add Convert To Links Modal #45984

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,208 @@
* WordPress dependencies
*/
import { edit } from '@wordpress/icons';
import { Button } from '@wordpress/components';
import { useDispatch } from '@wordpress/data';
import { forwardRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { forwardRef, useMemo, useState } from '@wordpress/element';
import { Button, Modal } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { createBlock as create } from '@wordpress/blocks';

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

// copied from packages/block-library/src/page-list/edit.js

// We only show the edit option when page count is <= MAX_PAGE_COUNT
// Performance of Navigation Links is not good past this value.
const MAX_PAGE_COUNT = 100;

const usePageData = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to its own file.

// 1. Grab editor settings
// 2. Call the selector when we need it
const { pages } = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );

return {
pages: getSettings().__experimentalFetchPageEntities( {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to test whether __experimentalFetchPageEntities is defined before attempting to utilise it.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example in the Post Editor this occurs

Screen Shot 2022-12-01 at 08 56 41

orderby: 'menu_order',
order: 'asc',
_fields: [ 'id', 'link', 'parent', 'title', 'menu_order' ],
per_page: -1,
context: 'view',
} ),
};
}, [] );

return useMemo( () => {
// TODO: Once the REST API supports passing multiple values to
// 'orderby', this can be removed.
// https://core.trac.wordpress.org/ticket/39037
const sortedPages = [ ...( pages ?? [] ) ].sort( ( a, b ) => {
if ( a.menu_order === b.menu_order ) {
return a.title.rendered.localeCompare( b.title.rendered );
}
return a.menu_order - b.menu_order;
} );
const pagesByParentId = sortedPages.reduce( ( accumulator, page ) => {
const { parent } = page;
if ( accumulator.has( parent ) ) {
accumulator.get( parent ).push( page );
} else {
accumulator.set( parent, [ page ] );
}
return accumulator;
}, new Map() );

return {
pages, // necessary for access outside the hook
pagesByParentId,
totalPages: pages?.length ?? null,
};
}, [ pages ] );
};

// copied from convert-to-links-modal.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can we have the full file path? 🙏

const convertSelectedBlockToNavigationLinks =
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to it's own file. All the more reason to do so in order to keep in sync with convert-to-links-modal.js

( { pages, clientId, replaceBlock, createBlock } ) =>
() => {
if ( ! pages?.length ) {
return;
}

const linkMap = {};
const navigationLinks = [];
pages.forEach( ( { id, title, link: url, type, parent } ) => {
// See if a placeholder exists. This is created if children appear before parents in list.
const innerBlocks = linkMap[ id ]?.innerBlocks ?? [];
linkMap[ id ] = createBlock(
'core/navigation-link',
{
id,
label: title.rendered,
url,
type,
kind: 'post-type',
},
innerBlocks
);

if ( ! parent ) {
navigationLinks.push( linkMap[ id ] );
} else {
if ( ! linkMap[ parent ] ) {
// Use a placeholder if the child appears before parent in list.
linkMap[ parent ] = { innerBlocks: [] };
}
const parentLinkInnerBlocks = linkMap[ parent ].innerBlocks;
parentLinkInnerBlocks.push( linkMap[ id ] );
}
} );

// Transform all links with innerBlocks into Submenus. This can't be done
// sooner because page objects have no information on their children.

const transformSubmenus = ( listOfLinks ) => {
listOfLinks.forEach( ( block, index, listOfLinksArray ) => {
const { attributes, innerBlocks } = block;
if ( innerBlocks.length !== 0 ) {
transformSubmenus( innerBlocks );
const transformedBlock = createBlock(
'core/navigation-submenu',
attributes,
innerBlocks
);
listOfLinksArray[ index ] = transformedBlock;
}
} );
};

transformSubmenus( navigationLinks );

replaceBlock( clientId, navigationLinks );
};

const ConvertToLinksModal = ( { onClose, clientId, pages } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is copied from somewhere can we have a comment to specific exactly where? 🙏

const hasPages = !! pages?.length;

const { replaceBlock } = useDispatch( blockEditorStore );

return (
<Modal
closeLabel={ __( 'Close' ) }
onRequestClose={ onClose }
title={ __( 'Customize this menu' ) }
className={ 'wp-block-page-list-modal' }
aria={ { describedby: 'wp-block-page-list-modal__description' } }
>
<p id={ 'wp-block-page-list-modal__description' }>
{ __(
'This menu is automatically kept in sync with pages on your site. You can manage the menu yourself by clicking customize below.'
) }
</p>
<div className="wp-block-page-list-modal-buttons">
<Button variant="tertiary" onClick={ onClose }>
{ __( 'Cancel' ) }
</Button>
<Button
variant="primary"
disabled={ ! hasPages }
onClick={ convertSelectedBlockToNavigationLinks( {
pages,
replaceBlock,
clientId,
createBlock: create,
} ) }
>
{ __( 'Customize' ) }
</Button>
</div>
</Modal>
);
};

export default forwardRef( function BlockEditButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I feel the BlockEditButton is now doing too much and has awareness of too many concepts.

Ideally these components need to stay as close to those in the canonical <ListView> as possible. This will make our lives much easier if we try to normalise them in the future.

To this end I think we should consider

  • moving <ConvertToLinksModal> modal to it's own file
  • extracting all pages logic to a custom hook
  • passing the requisite callbacks down into BlockEditButton as required.

We'll still be coupling concepts like Pages to the block editor package (not good) but at least it will be easier to extract and refactor in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look at the refactored version and let me know what you think. Moving usePageData and ConvertToLinksModal to their own files and lifting the onClick handler made the button small enough to make it irrelevant.

{ clientId, ...props },
ref
) {
const { selectBlock } = useDispatch( blockEditorStore );
const [ convertModalOpen, setConvertModalOpen ] = useState( false );
const { pages, totalPages } = usePageData();

const block = useSelect(
( select ) => {
return select( blockEditorStore ).getBlock( clientId );
},
[ clientId ]
);

const allowConvertToLinks =
'core/page-list' === block.name && totalPages <= MAX_PAGE_COUNT;
getdave marked this conversation as resolved.
Show resolved Hide resolved

const onClick = () => {
selectBlock( clientId );
georgeh marked this conversation as resolved.
Show resolved Hide resolved
if ( allowConvertToLinks ) {
setConvertModalOpen( ! convertModalOpen );
} else {
selectBlock( clientId );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this component should know anything about this concept. Lets lift this up and pass down as a prop. Perhaps if the onClick prop is defined then call that, else default to selectBlock?

};

return (
<Button { ...props } ref={ ref } icon={ edit } onClick={ onClick } />
<>
{ convertModalOpen && (
<ConvertToLinksModal
onClose={ () => setConvertModalOpen( false ) }
clientId={ clientId }
pages={ pages }
/>
) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be lifted up. I don't think it needs to "live" with the edit button.

<Button
{ ...props }
ref={ ref }
icon={ edit }
onClick={ onClick }
/>
</>
);
} );
18 changes: 17 additions & 1 deletion packages/edit-site/src/components/block-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,16 @@ export default function BlockEditor( { setIsInserterOpen } ) {
[ settingsBlockPatternCategories, restBlockPatternCategories ]
);

const { fetchPagesEntities } = useSelect( ( select ) => {
const { getEntityRecords } = select( coreStore );

return {
fetchPagesEntities: ( options = {} ) => {
return getEntityRecords( 'postType', 'page', options );
},
};
}, [] );

const settings = useMemo( () => {
const {
__experimentalAdditionalBlockPatterns,
Expand All @@ -138,8 +148,14 @@ export default function BlockEditor( { setIsInserterOpen } ) {
__unstableFetchMedia: fetchMedia,
__experimentalBlockPatterns: blockPatterns,
__experimentalBlockPatternCategories: blockPatternCategories,
__experimentalFetchPageEntities: fetchPagesEntities,
georgeh marked this conversation as resolved.
Show resolved Hide resolved
};
}, [ storedSettings, blockPatterns, blockPatternCategories ] );
}, [
storedSettings,
blockPatterns,
blockPatternCategories,
fetchPagesEntities,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's a chance this might cause a perf problem because fetchPagesEntities is always a new function reference?

I'm hoping the fact that useSelect has [] as deps means it will only run once and thus fetchPagesEntities will remain a consistent reference but if that's not true then it will cause problems as the useMemo will be invalidated for the entire editor settings.

] );

const [ blocks, onInput, onChange ] = useEntityBlockEditor(
'postType',
Expand Down