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

Site Editor: Fix template part area listing when a template has no edits #55085

Closed
wants to merge 4 commits into from
Closed

Site Editor: Fix template part area listing when a template has no edits #55085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b53f74
Site Editor: Fix template part area listing when a template has no edits
Mamaduka Oct 5, 2023
71a9e3d
Unit tests
Mamaduka Oct 5, 2023
f1115de
Update packages/edit-site/src/store/test/utils.js
Mamaduka Oct 6, 2023
8785d65
Fix unit tests
Mamaduka Oct 6, 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
2 changes: 1 addition & 1 deletion packages/edit-site/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ export const getCurrentTemplateTemplateParts = createRegistrySelector(
{ per_page: -1 }
);

return getFilteredTemplatePartBlocks( template.blocks, templateParts );
return getFilteredTemplatePartBlocks( template, templateParts );
}
);

Expand Down
45 changes: 31 additions & 14 deletions packages/edit-site/src/store/test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,6 @@ const FLATTENED_BLOCKS = [
},
];

const SINGLE_TEMPLATE_PART_BLOCK = {
clientId: '1',
name: 'core/template-part',
innerBlocks: [],
attributes: {
slug: 'aside',
theme: 'my-theme',
},
};

const TEMPLATE_PARTS = [
{
id: 'my-theme//header',
Expand All @@ -126,11 +116,32 @@ const TEMPLATE_PARTS = [
},
];

const TEMPLATE_NESTED_BLOCKS = {
blocks: NESTED_BLOCKS,
};

const TEMPLATE_SINGLE_BLOCK = {
blocks: [
{
clientId: '1',
name: 'core/template-part',
innerBlocks: [],
attributes: {
slug: 'aside',
theme: 'my-theme',
},
},
],
};

describe( 'utils', () => {
describe( 'getFilteredTemplatePartBlocks', () => {
it( 'returns a flattened list of filtered template parts preserving a depth-first order', () => {
const flattenedFilteredTemplateParts =
getFilteredTemplatePartBlocks( NESTED_BLOCKS, TEMPLATE_PARTS );
getFilteredTemplatePartBlocks(
TEMPLATE_NESTED_BLOCKS,
TEMPLATE_PARTS
);
expect( flattenedFilteredTemplateParts ).toEqual(
FLATTENED_BLOCKS
);
Expand All @@ -139,9 +150,15 @@ describe( 'utils', () => {
it( 'returns a cached result when passed the same params', () => {
// Clear the cache and call the function twice.
getFilteredTemplatePartBlocks.clear();
getFilteredTemplatePartBlocks( NESTED_BLOCKS, TEMPLATE_PARTS );
getFilteredTemplatePartBlocks(
TEMPLATE_NESTED_BLOCKS,
TEMPLATE_PARTS
);
expect(
getFilteredTemplatePartBlocks( NESTED_BLOCKS, TEMPLATE_PARTS )
getFilteredTemplatePartBlocks(
TEMPLATE_NESTED_BLOCKS,
TEMPLATE_PARTS
)
).toEqual( FLATTENED_BLOCKS );

// The function has been called twice with the same params, so the cache size should be 1.
Expand All @@ -157,7 +174,7 @@ describe( 'utils', () => {
// Call the function again, with different params.
expect(
getFilteredTemplatePartBlocks(
[ SINGLE_TEMPLATE_PART_BLOCK ],
TEMPLATE_SINGLE_BLOCK,
TEMPLATE_PARTS
)
).toEqual( [
Expand Down
19 changes: 14 additions & 5 deletions packages/edit-site/src/store/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,30 @@ import memoize from 'memize';
/**
* WordPress dependencies
*/
import { isTemplatePart } from '@wordpress/blocks';
import { isTemplatePart, parse } from '@wordpress/blocks';

const EMPTY_ARRAY = [];
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
function getBlocksFromRecord( record ) {
if ( record?.blocks ) {
return record?.blocks;
}

return record?.content && typeof record.content !== 'function'
? parse( record.content )
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't hit submit on my review comment for this line. I think this results in a clientId that doesn't match the existing blocks within the template, which means that the template area buttons no longer link back to the block in the template. I'm assuming a little here, so apologies if it's not what's causing it!

2023-10-06.12.15.09.mp4

In 6.3, the buttons link to the template part:

2023-10-06.12.21.51.mp4

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, @andrewserong! I forgot to test the Document settings sidebar and missed the issue.

You're right. Parsing the content separately will cause the clientId miss-match. I will start looking for a solution.

: [];
}

/**
* Get a flattened and filtered list of template parts and the matching block for that template part.
*
* Takes a list of blocks defined within a template, and a list of template parts, and returns a
* Takes a template, and a list of template parts, and returns a
* flattened list of template parts and the matching block for that template part.
*
* @param {Array} blocks Blocks to flatten.
* @param {Object} template Current template.
* @param {?Array} templateParts Available template parts.
* @return {Array} An array of template parts and their blocks.
*/
function getFilteredTemplatePartBlocks( blocks = EMPTY_ARRAY, templateParts ) {
function getFilteredTemplatePartBlocks( template, templateParts ) {
const blocks = getBlocksFromRecord( template );
const templatePartsById = templateParts
? // Key template parts by their ID.
templateParts.reduce(
Expand Down
Loading