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

Jump to bottom

Navigation: Try adding navigation link variants via server #29095

Merged
merged 12 commits into from
Mar 8, 2021
Merged

Navigation: Try adding navigation link variants via server #29095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2a2b518
Navigation Link: populate variation types like post link, page link, …
gwwar Feb 17, 2021
fb39f6b
favor changes in #29213
gwwar Feb 23, 2021
6e1a3fc
tidy up phpcs lint errors
gwwar Feb 19, 2021
9925d70
Add custom post type icon, courtesy of @jasmussen
gwwar Feb 22, 2021
ad227f6
Use item_link and item_link_description labels if available, use fall…
gwwar Feb 24, 2021
6533ea2
guard against $title and $description being undefined
gwwar Feb 25, 2021
6b07ad6
Use hardcoded fallback when server does not understand variations in …
gwwar Feb 26, 2021
2a65e2e
avoid import order change
gwwar Feb 26, 2021
9191e5e
do not render custom post type links if post is draft
gwwar Feb 26, 2021
7123e64
remove fallback label detection
gwwar Mar 1, 2021
5247f56
define isActive outside of map
gwwar Mar 1, 2021
b293fe6
Make sure page, post, tag, category variants are first
gwwar Mar 1, 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
3 changes: 3 additions & 0 deletions packages/block-library/src/navigation-link/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
},
"title": {
"type": "string"
},
"kind": {
"type": "string"
}
},
"usesContext": [
Expand Down
15 changes: 13 additions & 2 deletions packages/block-library/src/navigation-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,10 @@ const useIsDraggingWithin = ( elementRef ) => {
* /wp/v2/search.
*
* @param {string} type Link block's type attribute.
* @param {string} kind Link block's entity of kind (post-type|taxonomy)
* @return {{ type?: string, subtype?: string }} Search query params.
*/
function getSuggestionsQuery( type ) {
function getSuggestionsQuery( type, kind ) {
switch ( type ) {
case 'post':
case 'page':
Expand All @@ -115,6 +116,12 @@ function getSuggestionsQuery( type ) {
case 'tag':
return { type: 'term', subtype: 'post_tag' };
default:
if ( kind === 'taxonomy' ) {
return { type: 'term', subtype: type };
}
if ( kind === 'post-type' ) {
return { type: 'post', subtype: type };
}
return {};
}
}
Expand All @@ -137,6 +144,7 @@ export default function NavigationLinkEdit( {
description,
rel,
title,
kind,
} = attributes;
const link = {
url,
Expand Down Expand Up @@ -501,7 +509,10 @@ export default function NavigationLinkEdit( {
} }
noDirectEntry={ !! type }
noURLSuggestion={ !! type }
suggestionsQuery={ getSuggestionsQuery( type ) }
suggestionsQuery={ getSuggestionsQuery(
type,
kind
) }
onChange={ ( {
title: newTitle = '',
url: newURL = '',
Expand Down
11 changes: 8 additions & 3 deletions ...library/src/navigation-link/variations.js → ...rc/navigation-link/fallback-variations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ import {
postTitle as postIcon,
tag as tagIcon,
} from '@wordpress/icons';
const variations = [

// FALLBACK: this is only used when the server does not understand the variations property in the
// register_block_type_from_metadata call. see navigation-link/index.php.
// Delete this file when supported WP ranges understand the `variations` property when passed to
// register_block_type_from_metadata in index.php
const fallbackVariations = [
{
name: 'link',
isDefault: true,
Expand Down Expand Up @@ -51,10 +56,10 @@ const variations = [
* `isActive` function is used to find a variation match from a created
* Block by providing its attributes.
*/
variations.forEach( ( variation ) => {
fallbackVariations.forEach( ( variation ) => {
if ( variation.isActive ) return;
variation.isActive = ( blockAttributes, variationAttributes ) =>
blockAttributes.type === variationAttributes.type;
} );

export default variations;
export default fallbackVariations;
75 changes: 75 additions & 0 deletions packages/block-library/src/navigation-link/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import {
category,
page,
postTitle,
tag,
customPostType,
} from '@wordpress/icons';

/**
* Internal dependencies
*/
import fallbackVariations from './fallback-variations';

function getIcon( variationName ) {
switch ( variationName ) {
case 'post':
return postTitle;
case 'page':
return page;
case 'tag':
return tag;
case 'category':
return category;
default:
return customPostType;
}
}

function enhanceNavigationLinkVariations( settings, name ) {
if ( name !== 'core/navigation-link' ) {
return settings;
}

// Fallback handling may be deleted after supported WP ranges understand the `variations`
// property when passed to register_block_type_from_metadata in index.php
if ( ! settings.variations ) {
return {
...settings,
variations: fallbackVariations,
};
}

// Otherwise decorate server passed variations with an icon and isActive function
if ( settings.variations ) {
const isActive = ( blockAttributes, variationAttributes ) => {
return blockAttributes.type === variationAttributes.type;
};
const variations = settings.variations.map( ( variation ) => {
return {
...variation,
...( ! variation.icon && {
icon: getIcon( variation.name ),
} ),
...( ! variation.isActive && {
Copy link
Member

Choose a reason for hiding this comment

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

@gwwar, I was thinking about how we could eventually get rid of the filter that has to inject isActive to every variation defined on the server. I share my thoughts in #30739.

isActive,
} ),
};
} );
return {
...settings,
variations,
};
}
return settings;
}

addFilter(
Copy link
Member

@gziolo gziolo Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The fact that we register this filter as a side effect next to its definition isn't ideal but I guess it's fine since it's a special case. We could add this filter inside:

gutenberg/packages/block-library/src/index.js

Line 207 in 2a55b96

export const __experimentalRegisterExperimentalCoreBlocks =

Although it creates some indirection. The benefit would be that this code would be eliminated if the block isn't bundled. However, I assume that this block is going to be promoted to stable in WordPress 5.8 so 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 we might be able to avoid the icon handling if we say added string enum support for the @wordpress/icons package.

isActive is a bit trickier. Have we in practice seen different isActive functions for variant X and variant Y in the same variants list? We might be able to pull this up a level.

Although it creates some indirection. The benefit would be that this code would be eliminated if the block isn't bundled.

Oh interesting. I'd figure that the block already being behind the enableFSEBlocks flag would keep it out of the bundle. I'm open to experimenting with placement, though what it adds is probably pretty small.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's inevitable to use the filter on the client. I raised it mostly so you were aware of the implications it creates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gziolo Do folks mind if we keep this as is for now? The filter does run if we exclude the navigation link from the block library loader, but the alternative of adding it in __experimentalRegisterExperimentalCoreBlocks gets relatively messy.

'blocks.registerBlockType',
'core/navigation-link',
enhanceNavigationLinkVariations
);
11 changes: 8 additions & 3 deletions packages/block-library/src/navigation-link/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { InnerBlocks } from '@wordpress/block-editor';
import metadata from './block.json';
import edit from './edit';
import save from './save';
import variations from './variations';
import './hooks';

const { name } = metadata;

Expand All @@ -24,8 +24,6 @@ export const settings = {

description: __( 'Add a page, link, or another item to your navigation.' ),

variations,

__experimentalLabel: ( { label } ) => label,

merge( leftAttributes, { label: rightLabel = '' } ) {
Expand All @@ -39,6 +37,13 @@ export const settings = {

save,

example: {
attributes: {
label: _x( 'Example Link', 'navigation link preview example' ),
url: 'https://example.com',
},
},

deprecated: [
{
isEligible( attributes ) {
Expand Down
71 changes: 65 additions & 6 deletions packages/block-library/src/navigation-link/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,12 @@ function block_core_navigation_link_render_submenu_icon() {
* @return string Returns the post content with the legacy widget added.
*/
function render_block_core_navigation_link( $attributes, $content, $block ) {
$navigation_link_has_id = isset( $attributes['id'] ) && is_numeric( $attributes['id'] );
$is_post_type = isset( $attributes['kind'] ) && 'post-type' === $attributes['kind'];
$is_post_type = $is_post_type || isset( $attributes['type'] ) && ( 'post' === $attributes['type'] || 'page' === $attributes['type'] );

// Don't render the block's subtree if it is a draft.
if (
isset( $attributes['id'] ) &&
is_numeric( $attributes['id'] ) &&
isset( $attributes['type'] ) &&
( 'post' === $attributes['type'] || 'page' === $attributes['type'] )
) {
if ( $is_post_type && $navigation_link_has_id ) {
$post = get_post( $attributes['id'] );
if ( 'publish' !== $post->post_status ) {
return '';
Expand Down Expand Up @@ -226,17 +225,77 @@ function render_block_core_navigation_link( $attributes, $content, $block ) {
return $html;
}

/**
* Returns a navigation link variation
*
* @param WP_Taxonomy|WP_Post_Type $entity post type or taxonomy entity.
* @param string $kind string of value 'taxonomy' or 'post-type'.
*
* @return array
*/
function build_variation_for_navigation_link( $entity, $kind ) {
$name = 'post_tag' === $entity->name ? 'tag' : $entity->name;

$title = '';
$description = '';

if ( property_exists( $entity->labels, 'item_link' ) ) {
$title = $entity->labels->item_link;
}
if ( property_exists( $entity->labels, 'item_link_description' ) ) {
$description = $entity->labels->item_link_description;
}

return array(
'name' => $name,
'title' => $title,
'description' => $description,
'attributes' => array(
'type' => $name,
'kind' => $kind,
),
);
}

/**
* Register the navigation link block.
*
* @uses render_block_core_navigation()
* @throws WP_Error An WP_Error exception parsing the block definition.
*/
function register_block_core_navigation_link() {

$post_types = get_post_types( array( 'show_in_nav_menus' => true ), 'objects' );
$taxonomies = get_taxonomies( array( 'show_in_nav_menus' => true ), 'objects' );
$built_ins = array();
$variations = array();

if ( $post_types ) {
foreach ( $post_types as $post_type ) {
$variation = build_variation_for_navigation_link( $post_type, 'post-type' );
if ( 'post' === $variation['name'] || 'page' === $variation['name'] ) {
$built_ins[] = $variation;
} else {
$variations[] = $variation;
}
}
}
if ( $taxonomies ) {
foreach ( $taxonomies as $taxonomy ) {
$variation = build_variation_for_navigation_link( $taxonomy, 'taxonomy' );
if ( 'category' === $variation['name'] || 'tag' === $variation['name'] ) {
$built_ins[] = $variation;
} else {
$variations[] = $variation;
}
}
}

register_block_type_from_metadata(
__DIR__ . '/navigation-link',
array(
'render_callback' => 'render_block_core_navigation_link',
'variations' => array_merge( $built_ins, $variations ),
)
);
}
Expand Down
68 changes: 68 additions & 0 deletions phpunit/class-block-library-navigation-link-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ class Block_Library_Navigation_Link_Test extends WP_UnitTestCase {
private static $category;
private static $page;
private static $draft;
private static $custom_draft;
private static $custom_post;

private static $pages;
private static $terms;
Expand All @@ -31,6 +33,30 @@ public static function wpSetUpBeforeClass() {
);
self::$pages[] = self::$draft;

self::$custom_draft = self::factory()->post->create_and_get(
array(
'post_type' => 'cats',
'post_status' => 'draft',
'post_name' => 'metalcat',
'post_title' => 'Metal Cat',
'post_content' => 'Metal Cat content',
'post_excerpt' => 'Metal Cat',
)
);
self::$pages[] = self::$custom_draft;

self::$custom_post = self::factory()->post->create_and_get(
array(
'post_type' => 'dogs',
'post_status' => 'publish',
'post_name' => 'metaldog',
'post_title' => 'Metal Dog',
'post_content' => 'Metal Dog content',
'post_excerpt' => 'Metal Dog',
)
);
self::$pages[] = self::$custom_post;

self::$page = self::factory()->post->create_and_get(
array(
'post_type' => 'page',
Expand Down Expand Up @@ -166,4 +192,46 @@ function test_returns_link_for_plain_link() {
) !== false
);
}

function test_returns_empty_when_custom_post_type_draft() {
$page_id = self::$custom_draft->ID;

$parsed_blocks = parse_blocks(
"<!-- wp:navigation-link {\"label\":\"Draft Custom Post Type\",\"type\":\"cats\",\"kind\":\"post-type\",\"id\":{$page_id},\"url\":\"http://localhost:8888/?page_id={$page_id}\"} /-->"
);
$this->assertEquals( 1, count( $parsed_blocks ) );

$navigation_link_block = new WP_Block( $parsed_blocks[0], array() );

$this->assertEquals(
'',
gutenberg_render_block_core_navigation_link(
$navigation_link_block->attributes,
array(),
$navigation_link_block
)
);
}

function test_returns_link_when_custom_post_is_published() {
$page_id = self::$custom_post->ID;

$parsed_blocks = parse_blocks(
"<!-- wp:navigation-link {\"label\":\"Metal Dogs\",\"type\":\"dogs\",\"kind\":\"post-type\",\"id\":{$page_id},\"url\":\"http://localhost:8888/?page_id={$page_id}\"} /-->"
);
$this->assertEquals( 1, count( $parsed_blocks ) );

$navigation_link_block = new WP_Block( $parsed_blocks[0], array() );
$this->assertEquals(
true,
strpos(
gutenberg_render_block_core_navigation_link(
$navigation_link_block->attributes,
array(),
$navigation_link_block
),
'Metal Dogs'
) !== false
);
}
}