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

Add new API to allow inserter items to be prioritised #50510

Merged
merged 35 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
d3d0ed6
Proposing a way to sort items in the block inspector based on allowed…
scruffian May 10, 2023
1261536
Add inserterPriority API to inner blocks
getdave May 10, 2023
e8d2c78
Sort inserter based on inserterPriority prop from block list settings
getdave May 10, 2023
d72bb33
Use new inserterPriority API in Nav block
getdave May 10, 2023
66b4a02
Correct comment
getdave May 10, 2023
d987b29
Remove redundant prop
getdave May 10, 2023
4c1797e
Avoid stale inserterPriority
getdave May 11, 2023
d47e6c8
Make sorting function stable
getdave May 11, 2023
07aec20
Renaming
getdave May 11, 2023
25b18f2
Remove redundant comment
getdave May 11, 2023
264177a
remove spacer
scruffian May 11, 2023
509c362
Set prioritisedBlocks as empty array when no blockListSettings are fo…
jeryj May 11, 2023
1d81ef4
proritise -> prioritize for consistency
jeryj May 11, 2023
1523f1c
Update packages/block-editor/src/components/inner-blocks/use-nested-s…
scruffian May 11, 2023
eeb586d
Renaming constant to match updated name
jeryj May 11, 2023
be2e9d6
Add prioritizedKInnerBlocks to the inner-blocks README
jeryj May 11, 2023
5d6f1b5
lint fix
scruffian May 11, 2023
1162a72
update comment
scruffian May 11, 2023
51e5d3c
update comment
scruffian May 12, 2023
db4568f
pass the correct props to useNestedSettingsUpdate
scruffian May 12, 2023
96b70f3
Use stable ref
getdave May 12, 2023
21c12e1
Register the test Plugin for e2e tests
getdave May 12, 2023
9fde1a0
Register block with prioritzedInserterBlocks set
getdave May 12, 2023
ef93a6d
Add initial test scaffold
getdave May 12, 2023
b163447
Tidy up scaffolded test
getdave May 12, 2023
6bf6d37
Add test for new API
getdave May 12, 2023
8cb75c8
Try removing sort from helper
getdave May 12, 2023
d0ddf60
Fix test
getdave May 12, 2023
836b407
Add test to check does not override allowedBlocks when conflicted
getdave May 12, 2023
a954bae
Add additional assertion for retaining of correct number of results
getdave May 12, 2023
a787e9d
Ensure tests reflect target of Quick Inserter
getdave May 12, 2023
45bd5ac
sort allowed blocks on the tests that consume getAllBlockInserterItem…
MaggieCabrera May 12, 2023
01ad80a
Improve e2e test comment
getdave May 15, 2023
784bffb
Update packages/block-editor/src/components/inner-blocks/use-nested-s…
scruffian May 15, 2023
259a23d
Update packages/block-editor/src/components/inner-blocks/README.md
scruffian May 15, 2023
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
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/inner-blocks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,8 @@ For example, a button block, deeply nested in several levels of block `X` that u

- **Type:** `Function`
- **Default:** - `undefined`. The placeholder is an optional function that can be passed in to be a rendered component placed in front of the appender. This can be used to represent an example state prior to any blocks being placed. See the Social Links for an implementation example.

### `prioritizedInserterBlocks`
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to document this somewhere else or is this enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the official docs so I guess that's enough.


- **Type:** `Array`
- **Default:** - `undefined`. Determines which inner blocks should be returned first from the block inserter. For example, when inserting a block within the Navigation Block, `core/navigation-link` and `core/navigation-link/page` are the most commonly used inner blocks. We can use `prioritizedInserterBlocks` to pass these `navigation-link` blocks as an array so they can be returned first by default from the Navigation Block inserter.
scruffian marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/inner-blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function UncontrolledInnerBlocks( props ) {
const {
clientId,
allowedBlocks,
prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
template,
Expand All @@ -62,6 +63,7 @@ function UncontrolledInnerBlocks( props ) {
useNestedSettingsUpdate(
clientId,
allowedBlocks,
prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,13 @@ function UncontrolledInnerBlocks( props ) {
const {
clientId,
allowedBlocks,
prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
template,
templateLock,
templateInsertUpdatesSelection,
__experimentalCaptureToolbars: captureToolbars,
Comment on lines +77 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these ones?

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 this question.

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is "why are we adding all these extra props when this PR is about prioritizedInserterBlocks"?

orientation,
renderAppender,
renderFooterAppender,
Expand All @@ -97,7 +101,17 @@ function UncontrolledInnerBlocks( props ) {

const context = useBlockContext( clientId );

useNestedSettingsUpdate( clientId, allowedBlocks, templateLock );
useNestedSettingsUpdate(
clientId,
allowedBlocks,
prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
captureToolbars,
orientation,
layout
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this PR adding all these other settings and not only the core feature prioritizedInserterBlocks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because not doing that broke the Mobile implementation (cc @fluiddot).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool!

);

useInnerBlockTemplateSync(
clientId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const pendingSettingsUpdates = new WeakMap();
* @param {string} clientId The client ID of the block to update.
* @param {string[]} allowedBlocks An array of block names which are permitted
* in inner blocks.
* @param {string[]} prioritizedInserterBlocks Block names and/or block variations to be prioritized in the inserter, in the format {blockName}/{variationName}.
* @param {?WPDirectInsertBlock} __experimentalDefaultBlock The default block to insert: [ blockName, { blockAttributes } ].
* @param {?Function|boolean} __experimentalDirectInsert If a default block should be inserted directly by the
* appender.
Expand All @@ -40,6 +41,7 @@ const pendingSettingsUpdates = new WeakMap();
export default function useNestedSettingsUpdate(
clientId,
allowedBlocks,
prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
Expand All @@ -64,13 +66,27 @@ export default function useNestedSettingsUpdate(
[ clientId ]
);

// Memoize as inner blocks implementors often pass a new array on every
// render.
const _allowedBlocks = useMemo( () => allowedBlocks, allowedBlocks );
// Memoize allowedBlocks and prioritisedInnerBlocks based on the contents
// of the arrays. Implementors often pass a new array on every render,
// and the contents of the arrays are just strings, so the entire array
// can be passed as dependencies.

const _allowedBlocks = useMemo(
() => allowedBlocks,
// eslint-disable-next-line react-hooks/exhaustive-deps
allowedBlocks
);

const _prioritizedInserterBlocks = useMemo(
() => prioritizedInserterBlocks,
// eslint-disable-next-line react-hooks/exhaustive-deps
prioritizedInserterBlocks
);
Comment on lines +69 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Note: We'll have to remove this hack in the futere if we want to support dynamic allowedBlocks and prioritizedInserterBlocks. See #14515 (comment).


useLayoutEffect( () => {
const newSettings = {
allowedBlocks: _allowedBlocks,
prioritizedInserterBlocks: _prioritizedInserterBlocks,
templateLock:
templateLock === undefined || parentLock === 'contentOnly'
? parentLock
Expand Down Expand Up @@ -130,6 +146,7 @@ export default function useNestedSettingsUpdate(
clientId,
blockListSettings,
_allowedBlocks,
_prioritizedInserterBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
Expand Down
10 changes: 1 addition & 9 deletions packages/block-editor/src/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ class PrivateInserter extends Component {
prioritizePatterns,
onSelectOrClose,
selectBlockOnInsert,
orderInitialBlockItems,
} = this.props;

if ( isQuick ) {
Expand All @@ -174,7 +173,6 @@ class PrivateInserter extends Component {
isAppender={ isAppender }
prioritizePatterns={ prioritizePatterns }
selectBlockOnInsert={ selectBlockOnInsert }
orderInitialBlockItems={ orderInitialBlockItems }
/>
);
}
Expand Down Expand Up @@ -426,13 +424,7 @@ export const ComposedPrivateInserter = compose( [
] )( PrivateInserter );

const Inserter = forwardRef( ( props, ref ) => {
return (
<ComposedPrivateInserter
ref={ ref }
{ ...props }
orderInitialBlockItems={ undefined }
Copy link
Contributor

@talldan talldan May 11, 2023

Choose a reason for hiding this comment

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

I suppose the other option is to keep this API, but instead support passing a custom appender to List View (instead of the showAppender prop).

It still doesn't solve the issue where the block itself ends up with different ordering, unless both the block and the list view instance use the same custom appender, so I think what's in this PR is better. It's define once and it works everywhere which means there's less room for errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think

instead support passing a custom appender to List View (instead of the showAppender prop).

is better than the prop, but why would we need to keep orderInitialBlockItems?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only if you wanted specific instance X of an inserter to have a specific order.

The new implementation in this PR is a blanket application. Any block that uses the new prop on InnerBlocks no matter where it is with have items prioritised.

The previous implementation was more targetted.

Copy link
Contributor

@draganescu draganescu May 15, 2023

Choose a reason for hiding this comment

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

Wouldn't orderInitialBlockItems be the default order for when block's don't prioritize or for the generic canvas inserter in an empty paragraph?

Edit: no, that's for orderInitialBlockItems in the search results ro handle. This orderInitialBlockItems prop can be removed because it's effectively replaced by a public API doing the same thing.

/>
);
return <ComposedPrivateInserter ref={ ref } { ...props } />;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably follow up to remove the need for the private wrapper once (and if!) this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the private wrapper. This shape allows for easy future private APIs without all the boilerplate being re-added.

Copy link
Member

Choose a reason for hiding this comment

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

I was looking for why we haven't removed the private inserter. IMO, this doesn't seem like a good reason to keep things around just in case we might need them in the future.

} );

export default Inserter;
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export default function QuickInserter( {
isAppender,
prioritizePatterns,
selectBlockOnInsert,
orderInitialBlockItems,
} ) {
const [ filterValue, setFilterValue ] = useState( '' );
const [ destinationRootClientId, onInsertBlocks ] = useInsertionPoint( {
Expand Down Expand Up @@ -125,7 +124,6 @@ export default function QuickInserter( {
isDraggable={ false }
prioritizePatterns={ prioritizePatterns }
selectBlockOnInsert={ selectBlockOnInsert }
orderInitialBlockItems={ orderInitialBlockItems }
/>
</div>

Expand Down
55 changes: 49 additions & 6 deletions packages/block-editor/src/components/inserter/search-results.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { __, _n, sprintf } from '@wordpress/i18n';
import { VisuallyHidden } from '@wordpress/components';
import { useDebounce, useAsyncList } from '@wordpress/compose';
import { speak } from '@wordpress/a11y';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand All @@ -21,6 +22,7 @@ import useBlockTypesState from './hooks/use-block-types-state';
import { searchBlockItems, searchItems } from './search-items';
import InserterListbox from '../inserter-listbox';
import { orderBy } from '../../utils/sorting';
import { store as blockEditorStore } from '../../store';

const INITIAL_INSERTER_RESULTS = 9;
/**
Expand All @@ -31,6 +33,24 @@ const INITIAL_INSERTER_RESULTS = 9;
*/
const EMPTY_ARRAY = [];

const orderInitialBlockItems = ( items, priority ) => {
if ( ! priority ) {
return items;
}

items.sort( ( { id: aName }, { id: bName } ) => {
// Sort block items according to `priority`.
let aIndex = priority.indexOf( aName );
let bIndex = priority.indexOf( bName );
// All other block items should come after that.
if ( aIndex < 0 ) aIndex = priority.length;
if ( bIndex < 0 ) bIndex = priority.length;
return aIndex - bIndex;
} );

return items;
};

function InserterSearchResults( {
filterValue,
onSelect,
Expand All @@ -46,10 +66,22 @@ function InserterSearchResults( {
shouldFocusBlock = true,
prioritizePatterns,
selectBlockOnInsert,
orderInitialBlockItems,
} ) {
const debouncedSpeak = useDebounce( speak, 500 );

const { prioritizedBlocks } = useSelect(
( select ) => {
const blockListSettings =
select( blockEditorStore ).getBlockListSettings( rootClientId );

return {
prioritizedBlocks:
blockListSettings?.prioritizedInserterBlocks || EMPTY_ARRAY,
};
},
[ rootClientId ]
);

const [ destinationRootClientId, onInsertBlocks ] = useInsertionPoint( {
onSelect,
rootClientId,
Expand Down Expand Up @@ -89,10 +121,16 @@ function InserterSearchResults( {
if ( maxBlockTypesToShow === 0 ) {
return [];
}

let orderedItems = orderBy( blockTypes, 'frecency', 'desc' );
if ( ! filterValue && orderInitialBlockItems ) {
orderedItems = orderInitialBlockItems( orderedItems );

if ( ! filterValue && prioritizedBlocks.length ) {
orderedItems = orderInitialBlockItems(
orderedItems,
prioritizedBlocks
);
}

const results = searchBlockItems(
orderedItems,
blockTypeCategories,
Expand All @@ -108,8 +146,8 @@ function InserterSearchResults( {
blockTypes,
blockTypeCategories,
blockTypeCollections,
maxBlockTypes,
draganescu marked this conversation as resolved.
Show resolved Hide resolved
orderInitialBlockItems,
maxBlockTypesToShow,
prioritizedBlocks,
] );

// Announce search results on change.
Expand All @@ -124,7 +162,12 @@ function InserterSearchResults( {
count
);
debouncedSpeak( resultsFoundMessage );
}, [ filterValue, debouncedSpeak ] );
}, [
filterValue,
debouncedSpeak,
filteredBlockTypes,
filteredBlockPatterns,
] );

const currentShownBlockTypes = useAsyncList( filteredBlockTypes, {
step: INITIAL_INSERTER_RESULTS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,7 @@
import { useInstanceId } from '@wordpress/compose';
import { speak } from '@wordpress/a11y';
import { useSelect } from '@wordpress/data';
import {
forwardRef,
useState,
useEffect,
useCallback,
} from '@wordpress/element';
import { forwardRef, useState, useEffect } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';

/**
Expand All @@ -19,11 +14,6 @@ import { store as blockEditorStore } from '../../store';
import useBlockDisplayTitle from '../block-title/use-block-display-title';
import { ComposedPrivateInserter as PrivateInserter } from '../inserter';

const prioritizedInserterBlocks = [
'core/navigation-link/page',
'core/navigation-link',
];

export const Appender = forwardRef(
( { nestingLevel, blockCount, clientId, ...props }, ref ) => {
const [ insertedBlock, setInsertedBlock ] = useState( null );
Expand Down Expand Up @@ -68,19 +58,6 @@ export const Appender = forwardRef(
);
}, [ insertedBlockTitle ] );

const orderInitialBlockItems = useCallback( ( items ) => {
items.sort( ( { id: aName }, { id: bName } ) => {
// Sort block items according to `prioritizedInserterBlocks`.
let aIndex = prioritizedInserterBlocks.indexOf( aName );
let bIndex = prioritizedInserterBlocks.indexOf( bName );
// All other block items should come after that.
if ( aIndex < 0 ) aIndex = prioritizedInserterBlocks.length;
if ( bIndex < 0 ) bIndex = prioritizedInserterBlocks.length;
return aIndex - bIndex;
} );
return items;
}, [] );

if ( hideInserter ) {
return null;
}
Expand Down Expand Up @@ -110,7 +87,6 @@ export const Appender = forwardRef(
setInsertedBlock( maybeInsertedBlock );
}
} }
orderInitialBlockItems={ orderInitialBlockItems }
/>
<div
className="offcanvas-editor-appender__description"
Expand Down
5 changes: 5 additions & 0 deletions packages/block-library/src/navigation/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,8 @@ export const ALLOWED_BLOCKS = [
'core/navigation-submenu',
'core/loginout',
];

export const PRIORITIZED_INSERTER_BLOCKS = [
'core/navigation-link/page',
'core/navigation-link',
];
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ import { useMemo } from '@wordpress/element';
* Internal dependencies
*/
import PlaceholderPreview from './placeholder/placeholder-preview';
import { DEFAULT_BLOCK, ALLOWED_BLOCKS } from '../constants';
import {
DEFAULT_BLOCK,
ALLOWED_BLOCKS,
PRIORITIZED_INSERTER_BLOCKS,
} from '../constants';

export default function NavigationInnerBlocks( {
clientId,
Expand Down Expand Up @@ -93,6 +97,7 @@ export default function NavigationInnerBlocks( {
onInput,
onChange,
allowedBlocks: ALLOWED_BLOCKS,
prioritizedInserterBlocks: PRIORITIZED_INSERTER_BLOCKS,
__experimentalDefaultBlock: DEFAULT_BLOCK,
__experimentalDirectInsert: shouldDirectInsert,
orientation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ export async function getAllBlockInserterItemTitles() {
return inserterItem.innerText;
} );
} );
return [ ...new Set( inserterItemTitles ) ].sort();
return [ ...new Set( inserterItemTitles ) ];
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this in 8cb75c8.

Because it does sorting on the results the tests cannot assert on the priority of the blocks.

This helper is "doing it wrong". Instead it should return raw results and the consumer should do the sorting.

Let's see which tests break before deciding how to proceed from here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think this should be it's own sub PR?

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php
/**
* Plugin Name: Gutenberg Test InnerBlocks Prioritized Inserter Blocks
* Plugin URI: https://github.com/WordPress/gutenberg
* Author: Gutenberg Team
*
* @package gutenberg-test-inner-blocks-prioritized-inserter-blocks
*/

/**
* Registers a custom script for the plugin.
*/
function enqueue_inner_blocks_prioritized_inserter_blocks_script() {
wp_enqueue_script(
'gutenberg-test-inner-blocks-prioritized-inserter-blocks',
plugins_url( 'inner-blocks-prioritized-inserter-blocks/index.js', __FILE__ ),
array(
'wp-blocks',
'wp-block-editor',
'wp-element',
'wp-i18n',
),
filemtime( plugin_dir_path( __FILE__ ) . 'inner-blocks-prioritized-inserter-blocks/index.js' ),
true
);
}

add_action( 'init', 'enqueue_inner_blocks_prioritized_inserter_blocks_script' );
Loading