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

Patterns: Avoid mapping template parts objects to patterns #62927

Merged
merged 9 commits into from
Jun 28, 2024
Merged

Patterns: Avoid mapping template parts objects to patterns #62927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b093266
Patterns: Avoid mapping template parts objects to patterns
youknowriad Jun 27, 2024
7d1fa9b
Fix template part editor
youknowriad Jun 27, 2024
c5e0ad9
Fix reset action
youknowriad Jun 27, 2024
2ff18f3
Fix get title
youknowriad Jun 28, 2024
113c798
Small tweaks
youknowriad Jun 28, 2024
c66631d
Fix getItemId
youknowriad Jun 28, 2024
b939f56
Fix e2e test
youknowriad Jun 28, 2024
f0e4e0b
Small fix
youknowriad Jun 28, 2024
f57bb59
Fix duplicate action
youknowriad Jun 28, 2024
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
30 changes: 21 additions & 9 deletions packages/edit-site/src/components/page-patterns/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import { usePrevious } from '@wordpress/compose';
import { useEntityRecords } from '@wordpress/core-data';
import { privateApis as editorPrivateApis } from '@wordpress/editor';
import { privateApis as routerPrivateApis } from '@wordpress/router';
import { parse } from '@wordpress/blocks';
import { decodeEntities } from '@wordpress/html-entities';

/**
* Internal dependencies
Expand All @@ -53,6 +55,7 @@ import PatternsHeader from './header';
import { useLink } from '../routes/link';
import { useAddedBy } from '../page-templates/hooks';
import { useEditPostAction } from '../dataviews-actions';
import { defaultGetTitle } from './search-items';

const { ExperimentalBlockEditorProvider, useGlobalStyle } = unlock(
blockEditorPrivateApis
Expand Down Expand Up @@ -116,14 +119,21 @@ function Preview( { item, viewType } ) {
const descriptionId = useId();
const isUserPattern = item.type === PATTERN_TYPES.user;
const isTemplatePart = item.type === TEMPLATE_PART_POST_TYPE;
const isEmpty = ! item.blocks?.length;

const [ backgroundColor ] = useGlobalStyle( 'color.background' );
const { onClick } = useLink( {
postType: item.type,
postId: isUserPattern ? item.id : item.name,
postId: isUserPattern || isTemplatePart ? item.id : item.name,
canvas: 'edit',
} );
const blocks = useMemo( () => {
return (
item.blocks ??
oandregal marked this conversation as resolved.
Show resolved Hide resolved
parse( item.content.raw, {
__unstableSkipMigrationLogs: true,
} )
);
}, [ item?.content?.raw, item.blocks ] );
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
const isEmpty = ! blocks?.length;

return (
<div
Expand All @@ -140,7 +150,7 @@ function Preview( { item, viewType } ) {
{ ! isEmpty && (
<Async>
<BlockPreview
blocks={ item.blocks }
blocks={ blocks }
viewportWidth={ item.viewportWidth }
/>
</Async>
Expand Down Expand Up @@ -187,11 +197,13 @@ function Author( { item, viewType } ) {

function Title( { item } ) {
const isUserPattern = item.type === PATTERN_TYPES.user;
const isTemplatePart = item.type === TEMPLATE_PART_POST_TYPE;
const { onClick } = useLink( {
postType: item.type,
postId: isUserPattern ? item.id : item.name,
postId: isUserPattern || isTemplatePart ? item.id : item.name,
oandregal marked this conversation as resolved.
Show resolved Hide resolved
canvas: 'edit',
} );
const title = decodeEntities( defaultGetTitle( item ) );
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch 👍

Before:

Gravacao.do.ecra.2024-06-28.as.13.34.54.mov

After:

Gravacao.do.ecra.2024-06-28.as.13.32.12.mov

return (
<HStack alignment="center" justify="flex-start" spacing={ 2 }>
<Flex
Expand All @@ -201,7 +213,7 @@ function Title( { item } ) {
className="edit-site-patterns__pattern-title"
>
{ item.type === PATTERN_TYPES.theme ? (
item.title
title
) : (
<Button
variant="link"
Expand All @@ -210,7 +222,7 @@ function Title( { item } ) {
// See https://github.com/WordPress/gutenberg/pull/51898#discussion_r1243399243.
tabIndex="-1"
>
{ item.title || item.name }
{ title || item.name }
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
</Button>
) }
</Flex>
Expand Down Expand Up @@ -321,7 +333,7 @@ export default function DataviewsPatterns() {
_fields.push( {
header: __( 'Author' ),
id: 'author',
getValue: ( { item } ) => item.templatePart.author_text,
getValue: ( { item } ) => item.author_text,
render: ( { item } ) => {
return <Author viewType={ view.type } item={ item } />;
},
Expand Down Expand Up @@ -406,7 +418,7 @@ export default function DataviewsPatterns() {
fields={ fields }
actions={ actions }
data={ data || EMPTY_ARRAY }
getItemId={ ( item ) => item.name }
getItemId={ ( item ) => item.name ?? item.id }
oandregal marked this conversation as resolved.
Show resolved Hide resolved
isLoading={ isResolving }
view={ view }
onChangeView={ onChangeView }
Expand Down
7 changes: 5 additions & 2 deletions packages/edit-site/src/components/page-patterns/search-items.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@ import {
PATTERN_DEFAULT_CATEGORY,
PATTERN_USER_CATEGORY,
PATTERN_TYPES,
TEMPLATE_PART_POST_TYPE,
} from '../../utils/constants';

// Default search helpers.
const defaultGetName = ( item ) => item.name || '';
const defaultGetTitle = ( item ) => item.title;
const defaultGetName = ( item ) =>
item.type !== TEMPLATE_PART_POST_TYPE ? item.name || '' : '';
oandregal marked this conversation as resolved.
Show resolved Hide resolved
export const defaultGetTitle = ( item ) =>
typeof item.title === 'string' ? item.title : item.title.rendered;
const defaultGetDescription = ( item ) => item.description || '';
const defaultGetKeywords = ( item ) => item.keywords || [];
const defaultHasCategory = () => false;
Expand Down
40 changes: 6 additions & 34 deletions packages/edit-site/src/components/page-patterns/use-patterns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { parse } from '@wordpress/blocks';
import { useSelect, createSelector } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { store as editorStore } from '@wordpress/editor';
import { decodeEntities } from '@wordpress/html-entities';

/**
* Internal dependencies
Expand All @@ -16,7 +15,6 @@ import {
PATTERN_TYPES,
PATTERN_SYNC_TYPES,
TEMPLATE_PART_POST_TYPE,
TEMPLATE_ORIGINS,
TEMPLATE_PART_AREA_DEFAULT_CATEGORY,
} from '../../utils/constants';
import { unlock } from '../../lock-unlock';
Expand All @@ -25,38 +23,16 @@ import { store as editSiteStore } from '../../store';

const EMPTY_PATTERN_LIST = [];

const createTemplatePartId = ( theme, slug ) =>
theme && slug ? theme + '//' + slug : null;

const templatePartToPattern = ( templatePart ) => ( {
blocks: parse( templatePart.content.raw, {
Copy link
Member

Choose a reason for hiding this comment

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

Reviewed how this (or whether) is being used in the consumer code that uses parts (page-patterns). Everything has been migrated:

  • blocks: adapted consumer code
  • categories: unused
  • description: no need to do anything
  • isCustom: unused
  • keywords: unused
  • id: parts already have an id
  • name: removed & adapted the consumer code
  • title: removed & adapted the consumer code
  • type: as it is
  • _links: as it is

Copy link
Member

Choose a reason for hiding this comment

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

I see actions were also a consumer of this (through dataviews's item). I've only found one case that requires updating (title is not using decodedEntities https://github.com/WordPress/gutenberg/pull/62927/files#r1658485082 ) but the rest were already ported (isCustom, etc.).

__unstableSkipMigrationLogs: true,
} ),
categories: [ templatePart.area ],
description: templatePart.description || '',
isCustom: templatePart.source === TEMPLATE_ORIGINS.custom,
keywords: templatePart.keywords || [],
id: createTemplatePartId( templatePart.theme, templatePart.slug ),
name: createTemplatePartId( templatePart.theme, templatePart.slug ),
title: decodeEntities( templatePart.title.rendered ),
type: templatePart.type,
_links: templatePart._links,
templatePart,
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
} );

const selectTemplatePartsAsPatterns = createSelector(
const selectTemplateParts = createSelector(
( select, categoryId, search = '' ) => {
const { getEntityRecords, isResolving: isResolvingSelector } =
select( coreStore );
const { __experimentalGetDefaultTemplatePartAreas } =
select( editorStore );
const query = { per_page: -1 };
const rawTemplateParts =
const templateParts =
getEntityRecords( 'postType', TEMPLATE_PART_POST_TYPE, query ) ??
EMPTY_PATTERN_LIST;
const templateParts = rawTemplateParts.map( ( templatePart ) =>
templatePartToPattern( templatePart )
);

// In the case where a custom template part area has been removed we need
// the current list of areas to cross check against so orphaned template
Expand All @@ -66,12 +42,12 @@ const selectTemplatePartsAsPatterns = createSelector(

const templatePartHasCategory = ( item, category ) => {
if ( category !== TEMPLATE_PART_AREA_DEFAULT_CATEGORY ) {
return item.templatePart.area === category;
return item.area === category;
}

return (
item.templatePart.area === category ||
! templatePartAreas.includes( item.templatePart.area )
item.area === category ||
! templatePartAreas.includes( item.area )
);
};

Expand Down Expand Up @@ -298,11 +274,7 @@ export const usePatterns = (
return useSelect(
( select ) => {
if ( postType === TEMPLATE_PART_POST_TYPE ) {
return selectTemplatePartsAsPatterns(
select,
categoryId,
search
);
return selectTemplateParts( select, categoryId, search );
Copy link
Member

Choose a reason for hiding this comment

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

I've only found two uses of the usePatterns hook:

  • One is in the page-patterns screen, also adapted in this PR.
  • The other is in the sidebar, that doesn't need any adaption as it's only used to retrieve data from patterns but not from template parts post types (what is being modified in this PR).

Copy link
Member

Choose a reason for hiding this comment

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

I presume that, back in the days, the usePatterns hook was created as a way to avoid if conditions in the page (rules of hooks) and also to provide a stable structure to consumers (sidebar, page) independently from where the data came from. It seems to me the consumers still do a lot of data wrangling depending on the type, so not sure how effective this has been for the second goal.

It'd be good to rename it to usePatternsOrParts of something similar for clarity at some point: not a requirement for this PR.

} else if ( postType === PATTERN_TYPES.user && !! categoryId ) {
const appliedCategory =
categoryId === 'uncategorized' ? '' : categoryId;
Expand Down
43 changes: 23 additions & 20 deletions packages/editor/src/components/post-actions/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { __, _n, sprintf, _x } from '@wordpress/i18n';
import { store as noticesStore } from '@wordpress/notices';
import { useMemo, useState } from '@wordpress/element';
import { privateApis as patternsPrivateApis } from '@wordpress/patterns';
import { parse } from '@wordpress/blocks';

import {
Button,
Expand Down Expand Up @@ -52,17 +53,17 @@ function isTemplateRemovable( template ) {
// than the one returned from the endpoint. This is why we need to check for
// two props whether is custom or has a theme file.
return (
[ template.source, template.templatePart?.source ].includes(
TEMPLATE_ORIGINS.custom
) &&
! template.has_theme_file &&
! template.templatePart?.has_theme_file
template?.source === TEMPLATE_ORIGINS.custom &&
! template?.has_theme_file
);
}
const canDeleteOrReset = ( item ) => {
const isTemplatePart = item.type === TEMPLATE_PART_POST_TYPE;
const isUserPattern = item.type === PATTERN_TYPES.user;
return isUserPattern || ( isTemplatePart && item.isCustom );
return (
isUserPattern ||
( isTemplatePart && item.source === TEMPLATE_ORIGINS.custom )
);
};

function getItemTitle( item ) {
Expand Down Expand Up @@ -630,19 +631,13 @@ const renamePostAction = {
// two props whether is custom or has a theme file.
const isCustomPattern =
isUserPattern ||
( isTemplatePart &&
( post.isCustom || post.source === TEMPLATE_ORIGINS.custom ) );
const hasThemeFile =
isTemplatePart &&
( post.templatePart?.has_theme_file || post.has_theme_file );
( isTemplatePart && post.source === TEMPLATE_ORIGINS.custom );
const hasThemeFile = post?.has_theme_file;
return isCustomPattern && ! hasThemeFile;
},
RenderModal: ( { items, closeModal, onActionPerformed } ) => {
const [ item ] = items;
const originalTitle = decodeEntities(
typeof item.title === 'string' ? item.title : item.title.rendered
);
const [ title, setTitle ] = useState( () => originalTitle );
const [ title, setTitle ] = useState( () => getItemTitle( item ) );
const { editEntityRecord, saveEditedEntityRecord } =
useDispatch( coreStore );
const { createSuccessNotice, createErrorNotice } =
Expand Down Expand Up @@ -879,7 +874,7 @@ const isTemplatePartRevertable = ( item ) => {
if ( ! item ) {
return false;
}
const hasThemeFile = item.templatePart?.has_theme_file;
const hasThemeFile = item?.has_theme_file;
return canDeleteOrReset( item ) && hasThemeFile;
};

Expand Down Expand Up @@ -1031,26 +1026,34 @@ export const duplicateTemplatePartAction = {
modalHeader: _x( 'Duplicate template part', 'action label' ),
RenderModal: ( { items, closeModal } ) => {
const [ item ] = items;
const blocks = useMemo( () => {
return (
item.blocks ??
parse( item.content.raw, {
__unstableSkipMigrationLogs: true,
} )
);
}, [ item?.content?.raw, item.blocks ] );
const { createSuccessNotice } = useDispatch( noticesStore );
function onTemplatePartSuccess() {
createSuccessNotice(
sprintf(
// translators: %s: The new template part's title e.g. 'Call to action (copy)'.
__( '"%s" duplicated.' ),
item.title
getItemTitle( item )
),
{ type: 'snackbar', id: 'edit-site-patterns-success' }
);
closeModal();
}
return (
<CreateTemplatePartModalContents
blocks={ item.blocks }
defaultArea={ item.templatePart?.area || item.area }
blocks={ blocks }
defaultArea={ item.area }
defaultTitle={ sprintf(
/* translators: %s: Existing template part title */
__( '%s (Copy)' ),
item.title
getItemTitle( item )
) }
onCreate={ onTemplatePartSuccess }
onError={ closeModal }
Expand Down
7 changes: 1 addition & 6 deletions packages/editor/src/store/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,7 @@ export const revertTemplate =
export const removeTemplates =
( items ) =>
async ( { registry } ) => {
const isResetting = items.every(
( item ) =>
!! item &&
( item.has_theme_file ||
( item.templatePart && item.templatePart.has_theme_file ) )
);
const isResetting = items.every( ( item ) => item?.has_theme_file );

const promiseResult = await Promise.allSettled(
items.map( ( item ) => {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/site-editor/site-editor-url-navigation.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ test.describe( 'Site editor url navigation', () => {
.getByRole( 'region', {
name: 'Patterns content',
} )
.getByLabel( 'header', { exact: true } )
.getByText( 'header', { exact: true } )
.click();
await expect(
page.getByRole( 'region', { name: 'Editor content' } )
Expand Down
Loading