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

Classic Theme: Expose new Patterns page and remove Template Parts submenu #61080

Merged
merged 7 commits into from
May 1, 2024
27 changes: 27 additions & 0 deletions lib/compat/wordpress-6.6/compat.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php
/**
* WordPress 6.6 compatibility functions.
*
* @package gutenberg
*/

/**
* Change the Patterns submenu link and remove the Template Parts submenu for
* the Classic theme. This function should not be backported to core, and should be
* removed when the required WP core version for Gutenberg is >= 6.6.0.
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we have the wordpress-develop PR available, it might be handy to add a link to it here in this comment.

*
* @global array $submenu
*/
function gutenberg_change_patterns_link_and_remove_template_parts_submenu_item() {
if ( ! wp_is_block_theme() ) {
global $submenu;
foreach ( $submenu['themes.php'] as $key => $item ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The index of the Template Parts submenu varies depending on the core version. So we need to explore the submenu with foreach() (See this changeset).

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that this line produces PHP warnings.

[02-May-2024 08:18:23 UTC] PHP Warning:  Trying to access array offset on value of type null in ~/gutenberg/wp-content/plugins/gutenberg/lib/compat/wordpress-6.6/compat.php on line 18
[02-May-2024 08:18:23 UTC] PHP Warning:  foreach() argument must be of type array|object, null given in ~/gutenberg/wp-content/plugins/gutenberg/lib/compat/wordpress-6.6/compat.php on line 18

if ( 'edit.php?post_type=wp_block' === $item[2] ) {
$submenu['themes.php'][ $key ][2] = 'site-editor.php?path=/patterns';
} elseif ( 'site-editor.php?path=/wp_template_part/all' === $item[2] ) {
unset( $submenu['themes.php'][ $key ] );
}
}
}
}
add_action( 'admin_init', 'gutenberg_change_patterns_link_and_remove_template_parts_submenu_item' );
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.5/script-loader.php';

// WordPress 6.6 compat.
require __DIR__ . '/compat/wordpress-6.6/compat.php';
require __DIR__ . '/compat/wordpress-6.6/resolve-patterns.php';
require __DIR__ . '/compat/wordpress-6.6/block-bindings/pattern-overrides.php';
require __DIR__ . '/compat/wordpress-6.6/block-template-utils.php';
Expand Down
8 changes: 5 additions & 3 deletions packages/core-commands/src/admin-navigation-commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ import { privateApis as routerPrivateApis } from '@wordpress/router';
/**
* Internal dependencies
*/
import { useIsTemplatesAccessible, useIsBlockBasedTheme } from './hooks';
import { useIsTemplatesAccessible } from './hooks';
import { unlock } from './lock-unlock';

const { useHistory } = unlock( routerPrivateApis );

export function useAdminNavigationCommands() {
const history = useHistory();
const isTemplatesAccessible = useIsTemplatesAccessible();
const isBlockBasedTheme = useIsBlockBasedTheme();

const isSiteEditor = getPath( window.location.href )?.includes(
'site-editor.php'
Expand All @@ -45,7 +44,10 @@ export function useAdminNavigationCommands() {
label: __( 'Patterns' ),
icon: symbol,
callback: ( { close } ) => {
if ( isTemplatesAccessible && isBlockBasedTheme ) {
// The site editor and templates both check whether the user
// has edit_theme_options capabilities. We can leverage that here
// and omit the manage patterns link if the user can't access it.
if ( isTemplatesAccessible ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate this PR is only removing the isBlockBasedTheme check but do you think it is worth adding a comment here to explain why a Patterns command is relying on if templates are accessible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I just reused this comment:

// The site editor and templates both check whether the user
// has edit_theme_options capabilities. We can leverage that here
// and omit the manage patterns link if the user can't access it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure that comment captures the situation here.

The isTemplatesAccessible value is set via useIsTemplatesAccessible which checks read permissions for templates. Additionally, it should probably refer to this as a command rather than a link.

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 updated it with a0e4add, but I'm not sure if I'm communicating it well 😅

const args = {
path: '/patterns',
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,13 @@
*/
import { MenuItem } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { store as editorStore } from '@wordpress/editor';
import { useSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { addQueryArgs } from '@wordpress/url';

function ManagePatternsMenuItem() {
const url = useSelect( ( select ) => {
const { canUser } = select( coreStore );
const { getEditorSettings } = select( editorStore );

const isBlockTheme = getEditorSettings().__unstableIsBlockBasedTheme;
const defaultUrl = addQueryArgs( 'edit.php', {
post_type: 'wp_block',
} );
Expand All @@ -24,9 +20,7 @@ function ManagePatternsMenuItem() {
// The site editor and templates both check whether the user has
// edit_theme_options capabilities. We can leverage that here and not
// display the manage patterns link if the user can't access it.
return canUser( 'read', 'templates' ) && isBlockTheme
? patternsUrl
: defaultUrl;
return canUser( 'read', 'templates' ) ? patternsUrl : defaultUrl;
}, [] );

return (
Expand Down
39 changes: 20 additions & 19 deletions packages/edit-site/src/components/add-new-pattern/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ import { DropdownMenu } from '@wordpress/components';
import { useState, useRef } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
import { plus, symbol, symbolFilled, upload } from '@wordpress/icons';
import { useDispatch } from '@wordpress/data';
import { useSelect, useDispatch } from '@wordpress/data';
import { privateApis as routerPrivateApis } from '@wordpress/router';
import {
privateApis as editPatternsPrivateApis,
store as patternsStore,
} from '@wordpress/patterns';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';

/**
* Internal dependencies
Expand All @@ -30,7 +31,7 @@ const { CreatePatternModal, useAddPatternCategory } = unlock(
editPatternsPrivateApis
);

export default function AddNewPattern( { canCreateParts, canCreatePatterns } ) {
export default function AddNewPattern() {
const history = useHistory();
const { params } = useLocation();
const [ showPatternModal, setShowPatternModal ] = useState( false );
Expand All @@ -40,6 +41,10 @@ export default function AddNewPattern( { canCreateParts, canCreatePatterns } ) {
const { createSuccessNotice, createErrorNotice } =
useDispatch( noticesStore );
const patternUploadInputRef = useRef();
const isBlockBasedTheme = useSelect(
( select ) => select( coreStore ).getCurrentTheme()?.is_block_theme,
[]
);

function handleCreatePattern( { pattern, categoryId } ) {
setShowPatternModal( false );
Expand Down Expand Up @@ -71,31 +76,27 @@ export default function AddNewPattern( { canCreateParts, canCreatePatterns } ) {

const controls = [];

if ( canCreatePatterns ) {
controls.push( {
icon: symbol,
onClick: () => setShowPatternModal( true ),
title: __( 'Create pattern' ),
} );
}
controls.push( {
icon: symbol,
onClick: () => setShowPatternModal( true ),
title: __( 'Create pattern' ),
} );

if ( canCreateParts ) {
if ( isBlockBasedTheme ) {
controls.push( {
icon: symbolFilled,
onClick: () => setShowTemplatePartModal( true ),
title: __( 'Create template part' ),
} );
}

if ( canCreatePatterns ) {
controls.push( {
icon: upload,
onClick: () => {
patternUploadInputRef.current.click();
},
title: __( 'Import pattern from JSON' ),
} );
}
controls.push( {
icon: upload,
onClick: () => {
patternUploadInputRef.current.click();
},
title: __( 'Import pattern from JSON' ),
} );

const { categoryMap, findOrCreateTerm } = useAddPatternCategory();
return (
Expand Down
23 changes: 4 additions & 19 deletions packages/edit-site/src/components/layout/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,26 +117,11 @@ export default function useLayoutAreas() {
};
}

// Template parts
/*
* This is for legacy reasons, as the template parts are now part of the patterns screen.
* However, hybrid themes (classic themes that support template parts) still access this URL.
* While there are plans to make them use the patterns screen instead, we cannot do it for now.
* See discussion at https://github.com/WordPress/gutenberg/pull/60689
/* Patterns and Template Parts
* `/wp_template_part/all` path is no longer used, but uses Patterns page screens for
* backwards compatibility.
*/
if ( path === '/wp_template_part/all' ) {
return {
key: 'template-parts',
areas: {
sidebar: <SidebarNavigationScreenPatterns />,
content: <PagePatterns />,
mobile: <PagePatterns />,
},
};
}

// Patterns
if ( path === '/patterns' ) {
if ( path === '/patterns' || path === '/wp_template_part/all' ) {
return {
key: 'patterns',
areas: {
Expand Down
21 changes: 5 additions & 16 deletions packages/edit-site/src/components/page-patterns/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import {
LAYOUT_LIST,
PATTERN_TYPES,
TEMPLATE_PART_POST_TYPE,
TEMPLATE_PART_ALL_AREAS_CATEGORY,
PATTERN_SYNC_TYPES,
PATTERN_DEFAULT_CATEGORY,
ENUMERATION_TYPE,
Expand Down Expand Up @@ -253,21 +252,11 @@ function Title( { item, categoryId } ) {
}

export default function DataviewsPatterns() {
const {
categoryType,
categoryId: categoryIdFromURL,
path,
} = getQueryArgs( window.location.href );
const type =
categoryType ||
( path === '/wp_template_part/all'
? TEMPLATE_PART_POST_TYPE
: PATTERN_TYPES.theme );
const categoryId =
categoryIdFromURL ||
( path === '/wp_template_part/all'
? TEMPLATE_PART_ALL_AREAS_CATEGORY
: PATTERN_DEFAULT_CATEGORY );
const { categoryType, categoryId: categoryIdFromURL } = getQueryArgs(
window.location.href
);
const type = categoryType || PATTERN_TYPES.theme;
const categoryId = categoryIdFromURL || PATTERN_DEFAULT_CATEGORY;
const [ view, setView ] = useState( DEFAULT_VIEW );
const isUncategorizedThemePatterns =
type === PATTERN_TYPES.theme && categoryId === 'uncategorized';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { useDispatch, useSelect } from '@wordpress/data';
import { useDispatch } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { pencil } from '@wordpress/icons';
import { privateApis as routerPrivateApis } from '@wordpress/router';
Expand Down Expand Up @@ -30,28 +30,8 @@ export default function SidebarNavigationScreenPattern() {
useInitEditedEntityFromURL();

const patternDetails = usePatternDetails( postType, postId );
const isTemplatePartsMode = useSelect( ( select ) => {
return !! select( editSiteStore ).getSettings()
.supportsTemplatePartsMode;
}, [] );

/**
* This sidebar needs to temporarily accomodate two different "URLs" backpaths:
*
* 1. path = /patterns
* Block based themes. Also classic themes can access this URL, though it's not linked anywhere.
*
* 2. path = /wp_template_part/all
* Classic themes with support for block-template-parts. We need to list only Template Parts in this case.
* The URL is accessible from the Appearance > Template Parts menu.
*
* Depending on whether the theme supports block-template-parts, we go back to Patterns or Template screens.
* This is temporary. We aim to consolidate to /patterns.
*/
const backPath =
isTemplatePartsMode && postType === 'wp_template_part'
? { path: '/wp_template_part/all' }
: { path: '/patterns' };
const backPath = { path: '/patterns' };

return (
<SidebarNavigationScreen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,20 +107,12 @@ function CategoriesGroup( {
);
}

const EMPTY_ARRAY = [];
export default function SidebarNavigationScreenPatterns() {
const { categoryType, categoryId, path } = getQueryArgs(
window.location.href
);
const isTemplatePartsPath = path === '/wp_template_part/all';
const currentCategory =
categoryId ||
( isTemplatePartsPath
? TEMPLATE_PART_ALL_AREAS_CATEGORY
: PATTERN_DEFAULT_CATEGORY );
const currentType =
categoryType ||
( isTemplatePartsPath ? TEMPLATE_PART_POST_TYPE : PATTERN_TYPES.theme );
const currentCategory = categoryId || PATTERN_DEFAULT_CATEGORY;
const currentType = categoryType || PATTERN_TYPES.theme;

const { templatePartAreas, hasTemplateParts, isLoading } =
useTemplatePartAreas();
Expand All @@ -145,28 +137,11 @@ export default function SidebarNavigationScreenPatterns() {
return (
<SidebarNavigationScreen
isRoot={ ! isBlockBasedTheme }
title={
isTemplatePartsPath
? __( 'Manage template parts' )
: __( 'Patterns' )
}
description={
isTemplatePartsPath
? __(
'Create new template parts, or reset any customizations made to the template parts supplied by your theme.'
)
: __(
'Manage what patterns are available when editing the site.'
)
}
actions={
( isBlockBasedTheme || ! isTemplatePartsPath ) && (
<AddNewPattern
canCreateParts={ isBlockBasedTheme }
canCreatePatterns={ ! isTemplatePartsPath }
/>
)
}
title={ __( 'Patterns' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Just above this return statement, there's a reference to the /wp_template_part/all URL we need to update. Hybrid themes no longer have the "Appearance > Template Parts" link in the menu after this PR and we don't load different data depending on the URL either.

description={ __(
'Manage what patterns are available when editing the site.'
) }
actions={ <AddNewPattern /> }
content={
<>
{ isLoading && __( 'Loading items…' ) }
Expand All @@ -180,11 +155,7 @@ export default function SidebarNavigationScreenPatterns() {
<CategoriesGroup
path={ path }
templatePartAreas={ templatePartAreas }
patternCategories={
isTemplatePartsPath
? EMPTY_ARRAY
: patternCategories
}
patternCategories={ patternCategories }
currentCategory={ currentCategory }
currentType={ currentType }
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,6 @@ function useResolveEditedEntityAndContext( { path, postId, postType } ) {
return { isReady: true, postType: 'wp_template', postId, context };
}

if ( path === '/wp_template_part/all' && postId ) {
return { isReady: true, postType: 'wp_template_part', postId, context };
}
Comment on lines -229 to -231
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was added when DataViews was introduced to the Template Parts List page, so I think it's safe to remove it now (See #57952).


if ( postTypesWithoutParentTemplate.includes( postType ) ) {
return { isReady: true, postType, postId, context };
}
Expand Down
4 changes: 1 addition & 3 deletions packages/edit-site/src/utils/get-is-list-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ export default function getIsListPage(
isMobileViewport
) {
return (
[ '/wp_template', '/wp_template_part/all', '/pages' ].includes(
path
) ||
[ '/wp_template', '/pages' ].includes( path ) ||
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'm a little unsure about this change, but is it safe to remove it?

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 think it is.

By simply removing the wp_template_part/all path from this list, that page/route will be treated as if it is an "editor page" so it has commands like "Open styles" registered for it which will result in a broken black screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted by 5eb3d61

( path === '/patterns' &&
// Don't treat "/patterns" without categoryType and categoryId as a
// list page in mobile because the sidebar covers the whole page.
Expand Down