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

Block Editor: Optimize the 'useBlockDisplayTitle' hook #58250

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Block Editor: Optimize the 'useBlockDisplayTitle' hook #58250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
171 changes: 86 additions & 85 deletions packages/block-editor/src/components/block-title/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,76 +13,52 @@ import { useSelect } from '@wordpress/data';
*/
import BlockTitle from '../';

const blockTypeMap = {
'name-not-exists': null,
'name-exists': { title: 'Block Title' },
'name-with-label': { title: 'Block With Label' },
'name-with-custom-label': { title: 'Block With Custom Label' },
'name-with-long-label': { title: 'Block With Long Label' },
'reusable-block': { title: 'Reusable Block' },
};

const blockLabelMap = {
'Block With Label': 'Test Label',
'Block With Long Label':
'This is a longer label than typical for blocks to have.',
'Block With Custom Label': 'A Custom Label like a Block Variation Label',
};

jest.mock( '@wordpress/blocks', () => {
return {
getBlockType( name ) {
switch ( name ) {
case 'name-not-exists':
return null;

case 'name-exists':
return { title: 'Block Title' };

case 'name-with-label':
return { title: 'Block With Label' };

case 'name-with-custom-label':
return { title: 'Block With Custom Label' };

case 'name-with-long-label':
return { title: 'Block With Long Label' };
}
isReusableBlock( { title } ) {
return title === 'Reusable Block';
},
__experimentalGetBlockLabel( { title } ) {
switch ( title ) {
case 'Block With Label':
return 'Test Label';

case 'Block With Long Label':
return 'This is a longer label than typical for blocks to have.';

case 'Block With Custom Label':
return 'A Custom Label like a Block Variation Label';

default:
return title;
}
return blockLabelMap[ title ] || title;
},
};
} );

jest.mock( '../../use-block-display-information', () => {
const resultsMap = {
'id-name-exists': { title: 'Block Title' },
'id-name-with-label': { title: 'Block With Label' },
'id-name-with-long-label': { title: 'Block With Long Label' },
};
return jest.fn( ( clientId ) => resultsMap[ clientId ] );
} );

jest.mock( '@wordpress/data/src/components/use-select', () => {
// This allows us to tweak the returned value on each test.
const mock = jest.fn();
return mock;
} );
// This allows us to tweak the returned value on each test.
jest.mock( '@wordpress/data/src/components/use-select', () => jest.fn() );

describe( 'BlockTitle', () => {
it( 'renders nothing if name is falsey', () => {
useSelect.mockImplementation( () => ( {
name: null,
attributes: null,
} ) );
useSelect.mockImplementation( () => null );

const { container } = render( <BlockTitle /> );

expect( container ).toBeEmptyDOMElement();
} );

it( 'renders nothing if block type does not exist', () => {
useSelect.mockImplementation( () => ( {
name: 'name-not-exists',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-not-exists',
getBlockType: () => null,
} ) )
);

const { container } = render(
<BlockTitle clientId="afd1cb17-2c08-4e7a-91be-007ba7ddc3a1" />
Expand All @@ -92,45 +68,58 @@ describe( 'BlockTitle', () => {
} );

it( 'renders title if block type exists', () => {
useSelect.mockImplementation( () => ( {
name: 'name-exists',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-exists',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
getActiveBlockVariation: () => null,
} ) )
);

render( <BlockTitle clientId="id-name-exists" /> );

expect( screen.getByText( 'Block Title' ) ).toBeVisible();
} );

it( 'renders label if it is set', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-label',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-with-label',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
} ) )
);

render( <BlockTitle clientId="id-name-with-label" /> );

expect( screen.getByText( 'Test Label' ) ).toBeVisible();
} );

it( 'should prioritize reusable block title over title', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-label',
reusableBlockTitle: 'Reuse me!',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'reusable-block',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => ( { ref: 1 } ),
__experimentalGetReusableBlockTitle: () => 'Reuse me!',
} ) )
);

render( <BlockTitle clientId="id-name-with-label" /> );
render( <BlockTitle clientId="id-reusable-block" /> );

expect( screen.queryByText( 'Test Label' ) ).not.toBeInTheDocument();
expect( screen.getByText( 'Reuse me!' ) ).toBeVisible();
} );

it( 'should prioritize block label over title', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-custom-label',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-with-custom-label',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
} ) )
);

render( <BlockTitle clientId="id-name-with-label" /> );

Expand All @@ -140,22 +129,31 @@ describe( 'BlockTitle', () => {
).toBeVisible();
} );

it( 'should default to block information title if no reusable title or block name is available', () => {
useSelect.mockImplementation( () => ( {
name: 'some-rando-name',
attributes: null,
} ) );
it( 'should default to block variation title if no reusable title or block name is available', () => {
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-exists',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
getActiveBlockVariation: () => ( {
title: 'Block Variation Label',
} ),
} ) )
);

render( <BlockTitle clientId="id-name-with-label" /> );
render( <BlockTitle clientId="id-name-exists" /> );

expect( screen.getByText( 'Block With Label' ) ).toBeVisible();
expect( screen.getByText( 'Block Variation Label' ) ).toBeVisible();
} );

it( 'truncates the label with custom truncate length', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-long-label',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-with-long-label',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
} ) )
);

render(
<BlockTitle
Expand All @@ -168,10 +166,13 @@ describe( 'BlockTitle', () => {
} );

it( 'should not truncate the label if maximum length is undefined', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-long-label',
attributes: null,
} ) );
useSelect.mockImplementation( ( mapSelect ) =>
mapSelect( () => ( {
getBlockName: () => 'name-with-long-label',
getBlockType: ( name ) => blockTypeMap[ name ],
getBlockAttributes: () => null,
} ) )
);

render( <BlockTitle clientId="id-name-with-long-label" /> );

Expand Down
64 changes: 33 additions & 31 deletions packages/block-editor/src/components/block-title/use-block-display-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
*/
import { useSelect } from '@wordpress/data';
import {
getBlockType,
__experimentalGetBlockLabel as getBlockLabel,
isReusableBlock,
__experimentalGetBlockLabel as getBlockLabel,
store as blocksStore,
} from '@wordpress/blocks';

/**
* Internal dependencies
*/
import useBlockDisplayInformation from '../use-block-display-information';
import { store as blockEditorStore } from '../../store';

/**
Expand All @@ -35,49 +34,52 @@ export default function useBlockDisplayTitle( {
maximumLength,
context,
} ) {
const { attributes, name, reusableBlockTitle } = useSelect(
const blockTitle = useSelect(
( select ) => {
if ( ! clientId ) {
return {};
return null;
}

const {
getBlockName,
getBlockAttributes,
__experimentalGetReusableBlockTitle,
} = select( blockEditorStore );
const { getBlockType, getActiveBlockVariation } =
select( blocksStore );

const blockName = getBlockName( clientId );
if ( ! blockName ) {
return {};
const blockType = getBlockType( blockName );
if ( ! blockType ) {
return null;
}
const isReusable = isReusableBlock( getBlockType( blockName ) );
return {
attributes: getBlockAttributes( clientId ),
name: blockName,
reusableBlockTitle:
isReusable &&
__experimentalGetReusableBlockTitle(
getBlockAttributes( clientId ).ref
),
};

const attributes = getBlockAttributes( clientId );
const isReusable = isReusableBlock( blockType );
const reusableBlockTitle = isReusable
? __experimentalGetReusableBlockTitle( attributes.ref )
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably deprecate this selector and handle the pattern block title via __experimentalLabel.

cc @talldan, @kevin940726

Copy link
Member

Choose a reason for hiding this comment

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

Note that I'm adjusting this selector here Avoid fetching ALL reusable blocks (user patterns) on post/site editor load

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the correct PR link - #58239.

: null;

if ( reusableBlockTitle ) {
return reusableBlockTitle;
}

const label = getBlockLabel( blockType, attributes, context );
// If the label is defined we prioritize it over a possible block variation title match.
if ( label !== blockType.title ) {
return label;
}

const match = getActiveBlockVariation( blockName, attributes );
// Label will fallback to the title if no label is defined for the current label context.
return match?.title || blockType.title;
},
[ clientId ]
[ clientId, context ]
);

const blockInformation = useBlockDisplayInformation( clientId );
Copy link
Member

Choose a reason for hiding this comment

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

This is such a weird selector

Copy link
Member

Choose a reason for hiding this comment

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

In the sense that you might be getting a bunch of info that you don't need

if ( ! name || ! blockInformation ) {
if ( ! blockTitle ) {
return null;
}
const blockType = getBlockType( name );
const blockLabel = blockType
? getBlockLabel( blockType, attributes, context )
: null;

const label = reusableBlockTitle || blockLabel;
// Label will fallback to the title if no label is defined for the current
// label context. If the label is defined we prioritize it over a
// possible block variation title match.
const blockTitle =
label && label !== blockType.title ? label : blockInformation.title;

if (
maximumLength &&
Expand Down
Loading