Skip to content

Page List: add ability to convert to navigation links #30390

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

Merged
gwwar merged 8 commits into from
Apr 8, 2021
Merged

Page List: add ability to convert to navigation links #30390

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4dbea0c
Page List: add ability to convert to navigation links
gwwar Mar 30, 2021
1bb3a15
Polish.
jasmussen Apr 5, 2021
1fdf57f
Page List: only allow conversion when pages is <= 100. Disable conver…
gwwar Apr 6, 2021
c6a0268
Page List: add isBusy state when converting
gwwar Apr 6, 2021
5db1eb7
add aria-describedby to convert links modal
gwwar Apr 7, 2021
e35f132
update copy, add todos and explination on why ordering can differ fro…
gwwar Apr 8, 2021
4337b7e
fix some whitespace around comment
gwwar Apr 8, 2021
f70d114
phpcs fixes
gwwar Apr 8, 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
Prev Previous commit
Next Next commit
update copy, add todos and explination on why ordering can differ fro… 
…m render_callback vs rest api
  • Loading branch information
@gwwar
gwwar committed Apr 8, 2021
commit e35f1324f5058b589f030a6d23fe900729ac86b9
17 changes: 7 additions & 10 deletions packages/block-library/src/page-list/convert-to-links-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { Button, Modal } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useState } from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { store as coreDataStore } from '@wordpress/core-data';
import { createBlock as create } from '@wordpress/blocks';
Expand All @@ -17,23 +16,21 @@ export const convertSelectedBlockToNavigationLinks = ( {
clientId,
replaceBlock,
createBlock,
setIsConverting,
} ) => () => {
if ( ! pages ) {
return;
}

const linkMap = {};
const navigationLinks = [];
setIsConverting( true );
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, //TODO: raw or rendered?
label: title.rendered,
url,
type,
kind: 'post-type',
Expand All @@ -54,11 +51,9 @@ export const convertSelectedBlockToNavigationLinks = ( {
} );

replaceBlock( clientId, navigationLinks );
setIsConverting( false );
};

export default function ConvertToLinksModal( { onClose, clientId } ) {
const [ isConverting, setIsConverting ] = useState( false );
const { pages, pagesFinished } = useSelect(
( select ) => {
const { getEntityRecords, hasFinishedResolution } = select(
Expand All @@ -70,7 +65,11 @@ export default function ConvertToLinksModal( { onClose, clientId } ) {
{
per_page: MAX_PAGE_COUNT,
_fields: PAGE_FIELDS,
// TODO: When https://core.trac.wordpress.org/ticket/39037 REST API support for multiple orderby
// values is resolved, update 'orderby' to [ 'menu_order', 'post_title' ] to provide a consistent
// sort.
orderby: 'menu_order',
Copy link
Contributor

@tellthemachines tellthemachines Apr 8, 2021

Choose a reason for hiding this comment

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

Funny that this seems to return the pages in more or less oldest to most recent order, whereas the same parameter passed to get_pages returns a completely different order 😕

Copy link
Contributor Author

@gwwar gwwar Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is because get_pages has a default order of asc and WP_Query (used by the REST API) has a default order of desc, I'll double check which one we need to respect for menu_order.

It looks like we can't expect a stable sort from using menu_order, when items are equal to each other (eg menu_order of 0), so I'll need another dimension to sort on in addition to menu_order. Do folks prefer title alphabetizing or something like post date?

Copy link
Contributor

@jasmussen jasmussen Apr 8, 2021

Choose a reason for hiding this comment

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

For what it's worth, I don't think it's crucial to get this 100% right in this one PR. It's can be a nice enhancement in the future, but the value being able to convert the page list outweighs the downside that sorting might not be perfect, especially paired with the fact that you get immediate access to mover controls right after the conversion.

Copy link
Contributor Author

@gwwar gwwar Apr 8, 2021

Choose a reason for hiding this comment

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

Looks like it won't be possible to provide a consistent ordering until we can implement https://core.trac.wordpress.org/ticket/39037.

Fwiw, I implemented it that way initially because performance in the editor with >100 or so pages was pretty horrific

Good to know @tellthemachines! I'll look into adding multiple orderby support in the REST API next, then start on some general performance profiling for Navigation Links.

I've incorporated feedback and added a few explanations in e35f132

order: 'asc',
},
];
return {
Expand All @@ -95,7 +94,7 @@ export default function ConvertToLinksModal( { onClose, clientId } ) {
>
<p id={ 'wp-block-page-list-modal__description' }>
{ __(
'To edit this navigation menu, convert it to single page links. This allows you to add re-order, remove items, or edit their labels.'
'To edit this navigation menu, convert it to single page links. This allows you to add, re-order, remove items, or edit their labels.'
) }
</p>
<p>
Expand All @@ -109,14 +108,12 @@ export default function ConvertToLinksModal( { onClose, clientId } ) {
</Button>
<Button
isPrimary
isBusy={ isConverting }
disabled={ ! pagesFinished || isConverting }
disabled={ ! pagesFinished }
onClick={ convertSelectedBlockToNavigationLinks( {
pages,
replaceBlock,
clientId,
createBlock: create,
setIsConverting,
} ) }
>
{ __( 'Convert' ) }
Expand Down
9 changes: 8 additions & 1 deletion packages/block-library/src/page-list/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,14 @@ function render_block_core_page_list( $attributes, $content, $block ) {
static $block_id = 0;
$block_id++;

$all_pages = get_pages( array( 'sort_column' => 'menu_order' ) );
// TODO: When https://core.trac.wordpress.org/ticket/39037 REST API support for multiple orderby values is resolved,
// update 'sort_column' to 'menu_order, post_title'.
//
// Sorting by both menu_order and post_title ensures a stable sort. Otherwise with pages that have
// the same menu_order value, we can see different ordering depending on how DB queries are constructed
// internally. For example we might see a different order when a limit is set to <499 versus >= 500
//
$all_pages = get_pages( array( 'sort_column' => 'menu_order', 'order' => 'asc' ) );

$top_level_pages = array();

Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/page-list/test/convert-to-links-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,11 @@ describe( 'page list convert to links', () => {
innerBlocks,
} )
);
const setIsConverting = jest.fn();
const convertLinks = convertSelectedBlockToNavigationLinks( {
pages,
clientId: 'testId',
replaceBlock,
createBlock,
setIsConverting,
} );
convertLinks();
expect( replaceBlock.mock.calls?.[ 0 ]?.[ 1 ] ).toEqual( [
Expand Down Expand Up @@ -293,13 +291,11 @@ describe( 'page list convert to links', () => {
innerBlocks,
} )
);
const setIsConverting = jest.fn();
const convertLinks = convertSelectedBlockToNavigationLinks( {
pages,
clientId: 'testId',
replaceBlock,
createBlock,
setIsConverting,
} );
convertLinks();
expect( replaceBlock.mock.calls?.[ 0 ]?.[ 1 ] ).toEqual( [
Expand Down