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

Insert Links by default in Navigation block #34899

Merged
merged 11 commits into from
Sep 30, 2021
Merged

Insert Links by default in Navigation block #34899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b17bc8b
Insert Links by default in Navigation block
tellthemachines Sep 17, 2021
9c4f9c4
Fix errors.
tellthemachines Sep 20, 2021
ec29ec3
Open links by default with submenus
tellthemachines Sep 20, 2021
12fee33
Insert page item by default.
tellthemachines Sep 21, 2021
8c5f2ed
Fix re-rendering loop.
tellthemachines Sep 21, 2021
c7d2972
Actually fix re-rendering loop.
tellthemachines Sep 21, 2021
7068be9
Switch back to inserting generic link.
tellthemachines Sep 22, 2021
7a54865
Separate direct insert from default block props
tellthemachines Sep 23, 2021
b102fe4
Syntactic embellishment
tellthemachines Sep 28, 2021
3a69217
Update e2e tests and add a new test for Navigation block.
tellthemachines Sep 28, 2021
4e8457a
Try direct insert as a match function.
tellthemachines Sep 28, 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
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ function UncontrolledInnerBlocks( props ) {
const {
clientId,
allowedBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
template,
templateLock,
wrapperRef,
Expand All @@ -57,6 +59,8 @@ function UncontrolledInnerBlocks( props ) {
useNestedSettingsUpdate(
clientId,
allowedBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
captureToolbars,
orientation,
Expand Down
37 changes: 26 additions & 11 deletions packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,26 @@ import { getLayoutType } from '../../layouts';
* the block-editor store, then the store is updated with the new settings which
* came from props.
*
* @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} [templateLock] The template lock specified for the inner
* blocks component. (e.g. "all")
* @param {boolean} captureToolbars Whether or children toolbars should be shown
* in the inner blocks component rather than on
* the child block.
* @param {string} orientation The direction in which the block
* should face.
* @param {Object} layout The layout object for the block container.
* @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 {?Array} __experimentalDefaultBlock The default block to insert: [ blockName, { blockAttributes } ].
* @param {?Function|boolean} __experimentalDirectInsert If a default block should be inserted directly by the
* appender.
* @param {string} [templateLock] The template lock specified for the inner
* blocks component. (e.g. "all")
* @param {boolean} captureToolbars Whether or children toolbars should be shown
* in the inner blocks component rather than on
* the child block.
* @param {string} orientation The direction in which the block
* should face.
* @param {Object} layout The layout object for the block container.
*/
export default function useNestedSettingsUpdate(
clientId,
allowedBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
captureToolbars,
orientation,
Expand Down Expand Up @@ -83,13 +88,23 @@ export default function useNestedSettingsUpdate(
newSettings.orientation = layoutType.getOrientation( layout );
}

if ( __experimentalDefaultBlock !== undefined ) {
newSettings.__experimentalDefaultBlock = __experimentalDefaultBlock;
}

if ( __experimentalDirectInsert !== undefined ) {
newSettings.__experimentalDirectInsert = __experimentalDirectInsert;
}

if ( ! isShallowEqual( blockListSettings, newSettings ) ) {
updateBlockListSettings( clientId, newSettings );
}
}, [
clientId,
blockListSettings,
_allowedBlocks,
__experimentalDefaultBlock,
__experimentalDirectInsert,
templateLock,
parentLock,
captureToolbars,
Expand Down
18 changes: 15 additions & 3 deletions packages/block-editor/src/components/inserter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class Inserter extends Component {
disabled,
blockTitle,
hasSingleBlockType,
directInsertBlock,
toggleProps,
hasItems,
renderToggle = defaultRenderToggle,
Expand All @@ -113,6 +114,7 @@ class Inserter extends Component {
disabled: disabled || ! hasItems,
blockTitle,
hasSingleBlockType,
directInsertBlock,
toggleProps,
} );
}
Expand Down Expand Up @@ -168,12 +170,13 @@ class Inserter extends Component {
const {
position,
hasSingleBlockType,
directInsertBlock,
insertOnlyAllowedBlock,
__experimentalIsQuick: isQuick,
onSelectOrClose,
} = this.props;

if ( hasSingleBlockType ) {
if ( hasSingleBlockType || directInsertBlock?.length ) {
return this.renderToggle( { onToggle: insertOnlyAllowedBlock } );
}

Expand Down Expand Up @@ -202,6 +205,7 @@ export default compose( [
getBlockRootClientId,
hasInserterItems,
__experimentalGetAllowedBlocks,
__experimentalGetDirectInsertBlock,
} = select( blockEditorStore );
const { getBlockVariations } = select( blocksStore );

Expand All @@ -210,6 +214,10 @@ export default compose( [

const allowedBlocks = __experimentalGetAllowedBlocks( rootClientId );

const directInsertBlock = __experimentalGetDirectInsertBlock(
rootClientId
);

const hasSingleBlockType =
size( allowedBlocks ) === 1 &&
size(
Expand All @@ -226,6 +234,7 @@ export default compose( [
hasSingleBlockType,
blockTitle: allowedBlockType ? allowedBlockType.title : '',
allowedBlockType,
directInsertBlock,
rootClientId,
};
} ),
Expand All @@ -238,10 +247,11 @@ export default compose( [
isAppender,
hasSingleBlockType,
allowedBlockType,
directInsertBlock,
onSelectOrClose,
} = ownProps;

if ( ! hasSingleBlockType ) {
if ( ! hasSingleBlockType && ! directInsertBlock?.length ) {
return;
}

Expand Down Expand Up @@ -274,7 +284,9 @@ export default compose( [

const { insertBlock } = dispatch( blockEditorStore );

const blockToInsert = createBlock( allowedBlockType.name );
const blockToInsert = directInsertBlock?.length
? createBlock( ...directInsertBlock )
: createBlock( allowedBlockType.name );
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 the default block can work as an array or object. It may help to give a few more hints on the data structure with something like:

const [ name, attributes, innerBlocks ] = defaultBlock;
createBlock( name, attributes, innerBlocks );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you think of a scenario where we'd need defaultBlock to be an object? In the existing use cases (both this and your other PR) we're using it to store values to pass to createBlock so it makes more sense as an array.

In this PR, it's defined as an array in the jsdocs for the parameters passed to useNestedSettingsUpdate and the return value of __experimentalGetDirectInsertBlock. We'd need to change those if we wanted to support an object too.

Copy link
Contributor

@adamziel adamziel Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For me, a good reason to make it an object would be passing around a block instead of an array of arguments for createBlock. For example, this API could look like const DEFAULT_BLOCK = createBlock( 'core/navigation-link' ). One thing I don't like there is creating a uuid and a few other things we don't really need.

Maybe that could be a function then? For example const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-link' )

Copy link
Contributor

@gwwar gwwar Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 Good point, I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks. A function would make sense in that case, though we might want to pick a friendlier name. (I find that some folks get confused by the creator name). Maybe INSERT_DEFAULT_BLOCK or something similar?

Edit: Maybe a simple name like getDefaultBlock? One other piece of this is we'll likely need to modify the suggested items in the quick inserter as part of 34041. (The quick inserter is currently ordered by block frecency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not so sure about this. When allowedBlocks is in use, the block is created directly in the inserter. I'd prefer to emulate that logic: fetching a block type, and creating the block from that type in the inserter. Default block and allowed block shouldn't be too different, because they essentially serve the same purpose. The main difference is that only one default block may exist even if multiple blocks are allowed.

I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks

I'm not sure I follow. We shouldn't have any reason to pass innerBlocks to the inserter, because it should always insert a brand new block.

Copy link
Contributor

@adamziel adamziel Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks

I think @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:

const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-menu', {}, [
    createBlock( 'core/navigation-link', { title: "Home" }, [ ] ),
    createBlock( 'core/navigation-link', { title: "About" }, [ ] ),
] )

But as you say, I don't think it's a use-case that exists today.

Default block and allowed block shouldn't be too different, because they essentially serve the same purpose.

Hm I just thought of a completely different perspective here. Could we just make it a string to point out which supported block do we want to insert by default? It must be one of the supported blocks and we don't specify any attributes at the moment so maybe we don't need anything more? Or are we running in circles now?

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 @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:

That's right. A better example might be a container block that makes little sense without children, for example the buttons block.

Since we don't have an explicit case for passing though attributes or innerBlocks yet, I think it's fine deferring needing to pass through extra arguments until we need them. Whichever combination you prefer @tellthemachines

Copy link
Contributor

@gwwar gwwar Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

const blockToInsert = directInsertBlock?.length
					? createBlock( ...directInsertBlock )
					: createBlock( allowedBlockType.name )

One thing to note is that if folks pass through innerBlocks as the third item in an array, it's possible to have innerBlocks with static uuids. It's an edge case, but two options I can think of:

  1. clone the block
insertBlock( cloneBlock( blockToInsert ), getInsertionIndex(), rootClientId );
  1. Avoid passing through innerBlocks if we don't want to handle this case yet
const [ name, attributes ] = directInsertBlock;
createBlock( name, attributes )


insertBlock( blockToInsert, getInsertionIndex(), rootClientId );

Expand Down
33 changes: 33 additions & 0 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1792,6 +1792,39 @@ export const __experimentalGetAllowedBlocks = createSelector(
]
);

/**
* Returns the block to be directly inserted by the block appender.
*
* @param {Object} state Editor state.
* @param {?string} rootClientId Optional root client ID of block list.
*
* @return {?Array} The block type to be directly inserted.
*/
export const __experimentalGetDirectInsertBlock = createSelector(
( state, rootClientId = null ) => {
if ( ! rootClientId ) {
return;
}
const defaultBlock =
state.blockListSettings[ rootClientId ]?.__experimentalDefaultBlock;
const directInsert =
state.blockListSettings[ rootClientId ]?.__experimentalDirectInsert;
if ( ! defaultBlock || ! directInsert ) {
return;
}
if ( typeof directInsert === 'function' ) {
return directInsert( getBlock( state, rootClientId ) )
? defaultBlock
: null;
}
return defaultBlock;
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.blocks.tree[ rootClientId ],
]
);

const checkAllowListRecursive = ( blocks, allowedBlockTypes ) => {
if ( isBoolean( allowedBlockTypes ) ) {
return allowedBlockTypes;
Expand Down
4 changes: 4 additions & 0 deletions packages/block-library/src/navigation-submenu/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ import { name } from './block.json';

const ALLOWED_BLOCKS = [ 'core/navigation-link', 'core/navigation-submenu' ];

const DEFAULT_BLOCK = [ 'core/navigation-link' ];

const MAX_NESTING = 5;

/**
Expand Down Expand Up @@ -504,6 +506,8 @@ export default function NavigationSubmenuEdit( {
},
{
allowedBlocks: ALLOWED_BLOCKS,
__experimentalDefaultBlock: DEFAULT_BLOCK,
__experimentalDirectInsert: true,
renderAppender:
isSelected ||
( isImmediateParentOfSelectedBlock &&
Expand Down
12 changes: 12 additions & 0 deletions packages/block-library/src/navigation/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ const ALLOWED_BLOCKS = [
'core/navigation-submenu',
];

const DEFAULT_BLOCK = [ 'core/navigation-link' ];

const DIRECT_INSERT = ( block ) => {
return block.innerBlocks.every(
( { name } ) =>
name === 'core/navigation-link' ||
name === 'core/navigation-submenu'
);
};

const LAYOUT = {
type: 'default',
alignments: [],
Expand Down Expand Up @@ -164,6 +174,8 @@ function Navigation( {
},
{
allowedBlocks: ALLOWED_BLOCKS,
__experimentalDefaultBlock: DEFAULT_BLOCK,
__experimentalDirectInsert: DIRECT_INSERT,
orientation: attributes.orientation,
renderAppender: CustomAppender || appender,

Expand Down
10 changes: 10 additions & 0 deletions packages/e2e-tests/specs/experiments/blocks/__snapshots__/navigation.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ exports[`Navigation Creating from existing Pages allows a navigation block to be
<!-- /wp:navigation -->"
`;

exports[`Navigation Shows the quick inserter when the block contains non-navigation specific blocks 1`] = `
"<!-- wp:navigation -->
<!-- wp:navigation-link {\\"label\\":\\"WP\\",\\"url\\":\\"https://wordpress.org\\",\\"kind\\":\\"custom\\",\\"isTopLevelLink\\":true} /-->

<!-- wp:site-title /-->

<!-- wp:navigation-link {\\"label\\":\\"WP News\\",\\"url\\":\\"https://wordpress.org/news/\\",\\"kind\\":\\"custom\\",\\"isTopLevelLink\\":true} /-->
<!-- /wp:navigation -->"
`;

exports[`Navigation allows an empty navigation block to be created and manually populated using a mixture of internal and external links 1`] = `
"<!-- wp:navigation -->
<!-- wp:navigation-link {\\"label\\":\\"WP\\",\\"url\\":\\"https://wordpress.org\\",\\"kind\\":\\"custom\\",\\"isTopLevelLink\\":true} /-->
Expand Down
61 changes: 45 additions & 16 deletions packages/e2e-tests/specs/experiments/blocks/navigation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,6 @@ async function createEmptyNavBlock() {
await startEmptyButton.click();
}

async function addLinkBlock() {
// Using 'click' here checks for regressions of https://github.com/WordPress/gutenberg/issues/18329,
// an issue where the block appender requires two clicks.
await page.click( '.wp-block-navigation .block-list-appender' );

const [ linkButton ] = await page.$x(
"//*[contains(@class, 'block-editor-inserter__quick-inserter')]//*[text()='Custom Link']"
);
await linkButton.click();
}

async function toggleSidebar() {
await page.click(
'.edit-post-header__settings button[aria-label="Settings"]'
Expand Down Expand Up @@ -414,7 +403,7 @@ describe( 'Navigation', () => {

await createEmptyNavBlock();

await addLinkBlock();
await page.click( '.wp-block-navigation .block-list-appender' );

// Add a link to the Link block.
await updateActiveNavigationLink( {
Expand All @@ -425,7 +414,7 @@ describe( 'Navigation', () => {

await showBlockToolbar();

await addLinkBlock();
await page.click( '.wp-block-navigation .block-list-appender' );

// After adding a new block, search input should be shown immediately.
// Verify that Escape would close the popover.
Expand Down Expand Up @@ -477,7 +466,7 @@ describe( 'Navigation', () => {

await createEmptyNavBlock();

await addLinkBlock();
await page.click( '.wp-block-navigation .block-list-appender' );

// Add a link to the Link block.
await updateActiveNavigationLink( {
Expand All @@ -487,7 +476,7 @@ describe( 'Navigation', () => {

await showBlockToolbar();

await addLinkBlock();
await page.click( '.wp-block-navigation .block-list-appender' );

// Wait for URL input to be focused
await page.waitForSelector(
Expand Down Expand Up @@ -548,7 +537,7 @@ describe( 'Navigation', () => {
// Create an empty nav block.
await createEmptyNavBlock();

await addLinkBlock();
await page.click( '.wp-block-navigation .block-list-appender' );

// Wait for URL input to be focused
await page.waitForSelector(
Expand Down Expand Up @@ -642,6 +631,46 @@ describe( 'Navigation', () => {
expect( navSubmenusLength ).toEqual( navButtonTogglesLength );
} );

it( 'Shows the quick inserter when the block contains non-navigation specific blocks', async () => {
// Add the navigation block.
await insertBlock( 'Navigation' );

// Create an empty nav block.
await page.waitForSelector( '.wp-block-navigation-placeholder' );

await createEmptyNavBlock();

// Add a Link block first.
await page.click( '.wp-block-navigation .block-list-appender' );

// Add a link to the Link block.
await updateActiveNavigationLink( {
url: 'https://wordpress.org',
label: 'WP',
type: 'url',
} );

// Now add a different block type.
await insertBlock( 'Site Title' );

// Now try inserting another Link block via the quick inserter.
await page.click( '.wp-block-navigation .block-list-appender' );

const [ linkButton ] = await page.$x(
"//*[contains(@class, 'block-editor-inserter__quick-inserter')]//*[text()='Custom Link']"
);
await linkButton.click();

await updateActiveNavigationLink( {
url: 'https://wordpress.org/news/',
label: 'WP News',
type: 'url',
} );

// Expect a Navigation block with two links and a Site Title.
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

// The following tests are unstable, roughly around when https://github.com/WordPress/wordpress-develop/pull/1412
// landed. The block manually tests well, so let's skip to unblock other PRs and immediately follow up. cc @vcanales
it.skip( 'loads frontend code only if the block is present', async () => {
Expand Down
5 changes: 0 additions & 5 deletions packages/e2e-tests/specs/experiments/navigation-editor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,11 +380,6 @@ describe( 'Navigation editor', () => {
);
await appender.click();

const linkInserterItem = await page.waitForXPath(
'//button[@role="option"]//span[.="Custom Link"]'
);
await linkInserterItem.click();

await page.waitForSelector( 'input[aria-label="URL"]' );

// The link suggestions should be searchable.
Expand Down