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

Page List: Respect the selected parent page when converting to a list of navigation links #47651

Merged
merged 3 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 6 additions & 5 deletions packages/block-library/src/page-list/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,6 @@ export default function PageListEdit( {
pages?.length > 0 &&
pages?.length <= MAX_PAGE_COUNT;

const convertToNavigationLinks = useConvertToNavigationLinks( {
clientId,
pages,
} );

const pagesByParentId = useMemo( () => {
if ( pages === null ) {
return new Map();
Expand All @@ -213,6 +208,12 @@ export default function PageListEdit( {
}, new Map() );
}, [ pages ] );

const convertToNavigationLinks = useConvertToNavigationLinks( {
clientId,
pages,
parentPageID,
} );

const blockProps = useBlockProps( {
className: classnames( 'wp-block-page-list', {
'has-text-color': !! context.textColor,
Expand Down
134 changes: 134 additions & 0 deletions packages/block-library/src/page-list/test/convert-to-links-modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -383,5 +383,139 @@ describe( 'page list convert to links', () => {
},
] );
} );

it( 'Can use a different parent page', () => {
const pages = [
{
title: {
raw: 'Sample Page',
rendered: 'Sample Page',
},
id: 2,
parent: 0,
link: 'http://wordpress.local/sample-page/',
type: 'page',
},
{
title: {
raw: 'About',
rendered: 'About',
},
id: 34,
parent: 0,
link: 'http://wordpress.local/about/',
type: 'page',
},
{
title: {
raw: 'Contact Page',
rendered: 'Contact Page',
},
id: 37,
parent: 0,
link: 'http://wordpress.local/contact-page/',
type: 'page',
},
{
title: {
raw: 'Test',
rendered: 'Test',
},
id: 229,
parent: 0,
link: 'http://wordpress.local/test/',
type: 'page',
},
{
title: {
raw: 'About Sub 1',
rendered: 'About Sub 1',
},
id: 738,
parent: 34,
link: 'http://wordpress.local/about/about-sub-1/',
type: 'page',
},
{
title: {
raw: 'About Sub 2',
rendered: 'About Sub 2',
},
id: 740,
parent: 34,
link: 'http://wordpress.local/about/about-sub-2/',
type: 'page',
},
{
title: {
raw: 'Test Sub',
rendered: 'Test Sub',
},
id: 742,
parent: 229,
link: 'http://wordpress.local/test/test-sub/',
type: 'page',
},
{
title: {
raw: 'Test Sub Sub',
rendered: 'Test Sub Sub',
},
id: 744,
parent: 742,
link: 'http://wordpress.local/test/test-sub/test-sub-sub/',
type: 'page',
},
];

const convertLinksWithParentOneLevel = convertToNavigationLinks(
pages,
34
);

expect( convertLinksWithParentOneLevel ).toEqual( [
{
attributes: {
id: 738,
kind: 'post-type',
label: 'About Sub 1',
type: 'page',
url: 'http://wordpress.local/about/about-sub-1/',
},
innerBlocks: [],
name: 'core/navigation-link',
},
{
attributes: {
id: 740,
kind: 'post-type',
label: 'About Sub 2',
type: 'page',
url: 'http://wordpress.local/about/about-sub-2/',
},
innerBlocks: [],
name: 'core/navigation-link',
},
] );

const convertLinksWithParentTwoLevels = convertToNavigationLinks(
pages,
742
);

expect( convertLinksWithParentTwoLevels ).toEqual( [
{
attributes: {
id: 744,
kind: 'post-type',
label: 'Test Sub Sub',
type: 'page',
url: 'http://wordpress.local/test/test-sub/test-sub-sub/',
},
innerBlocks: [],
name: 'core/navigation-link',
},
] );
} );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ import { createBlock } from '@wordpress/blocks';
import { useSelect, useDispatch } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';

export function convertToNavigationLinks( pages = [] ) {
/**
* Converts an array of pages into a nested array of navigation link blocks.
*
* @param {Array} pages An array of pages.
*
* @return {Array} A nested array of navigation link blocks.
*/
function createNavigationLinks( pages = [] ) {
const linkMap = {};
const navigationLinks = [];
pages.forEach( ( { id, title, link: url, type, parent } ) => {
Expand All @@ -30,11 +37,61 @@ export function convertToNavigationLinks( pages = [] ) {
// Use a placeholder if the child appears before parent in list.
linkMap[ parent ] = { innerBlocks: [] };
}
// Although these variables are not referenced, they are needed to store the innerBlocks in memory.
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 understand: how are they useful if nobody reads them?

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 100% get it either. I think it means they are stored in memory, so that they don't get lost when a mutation happens. This is old code that might be possible to write in a clearer way, but I think that's outside of the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you remove these lines it breaks :)

const parentLinkInnerBlocks = linkMap[ parent ].innerBlocks;
parentLinkInnerBlocks.push( linkMap[ id ] );
}
} );

return navigationLinks;
}

/**
* Finds a navigation link block by id, recursively.
* It might be possible to make this a more generic helper function.
*
* @param {Array} navigationLinks An array of navigation link blocks.
* @param {number} id The id of the navigation link to find.
*
* @return {Object|null} The navigation link block with the given id.
*/
function findNavigationLinkById( navigationLinks, id ) {
for ( const navigationLink of navigationLinks ) {
// Is this the link we're looking for?
if ( navigationLink.attributes.id === id ) {
return navigationLink;
}

// If not does it have innerBlocks?
if ( navigationLink.innerBlocks && navigationLink.innerBlocks.length ) {
const foundNavigationLink = findNavigationLinkById(
navigationLink.innerBlocks,
id
);

if ( foundNavigationLink ) {
return foundNavigationLink;
}
}
}

return null;
}

export function convertToNavigationLinks( pages = [], parentPageID = null ) {
let navigationLinks = createNavigationLinks( pages );

// If a parent page ID is provided, only return the children of that page.
if ( parentPageID ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I wonder if we should not instead filter pages and use createNavigationLinks on the filtered pages?

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 thought about that, but it's technically more complex as pages isn't a nested array, so we'd have to filter on a parent id, and then all the parents of that id, and then all the parents of that one etc. I think this approach is simpler.

const parentPage = findNavigationLinkById(
navigationLinks,
parentPageID
);
if ( parentPage && parentPage.innerBlocks ) {
navigationLinks = parentPage.innerBlocks;
}
}

// 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 ) => {
Expand All @@ -53,11 +110,14 @@ export function convertToNavigationLinks( pages = [] ) {
};

transformSubmenus( navigationLinks );

return navigationLinks;
}

export function useConvertToNavigationLinks( { clientId, pages } ) {
export function useConvertToNavigationLinks( {
clientId,
pages,
parentPageID,
} ) {
const { replaceBlock, selectBlock } = useDispatch( blockEditorStore );

const { parentNavBlockClientId } = useSelect(
Expand All @@ -79,7 +139,7 @@ export function useConvertToNavigationLinks( { clientId, pages } ) {
);

return () => {
const navigationLinks = convertToNavigationLinks( pages );
const navigationLinks = convertToNavigationLinks( pages, parentPageID );

// Replace the Page List block with the Navigation Links.
replaceBlock( clientId, navigationLinks );
Expand Down