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

Use blockLabel for BlockTitle component #23847

Merged
merged 6 commits into from
Aug 13, 2020
Merged

Use blockLabel for BlockTitle component #23847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1671622
Use blockLabel for BlockTitle component
noahtallen Jul 9, 2020
1957e5f
Use dash as separator
noahtallen Jul 15, 2020
f484165
Check if label does not equal title, use colon
noahtallen Jul 15, 2020
27aa6c0
Fix typo in comment
noahtallen Aug 12, 2020
9cdf64c
Update BlockTitle unit tests
noahtallen Aug 12, 2020
60cd2e5
Fix possible error when clientId did not exist
noahtallen Aug 13, 2020
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
33 changes: 27 additions & 6 deletions packages/block-editor/src/components/block-title/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
/**
* External dependencies
*/
import { truncate } from 'lodash';

/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { getBlockType } from '@wordpress/blocks';
import {
getBlockType,
__experimentalGetBlockLabel as getBlockLabel,
} from '@wordpress/blocks';

/**
* Renders the block's configured title as a string, or empty if the title
Expand All @@ -20,13 +28,18 @@ import { getBlockType } from '@wordpress/blocks';
* @return {?string} Block title.
*/
export default function BlockTitle( { clientId } ) {
const name = useSelect(
const { attributes, name } = useSelect(
( select ) => {
if ( ! clientId ) {
return null;
return {};
}
const { getBlockName } = select( 'core/block-editor' );
return getBlockName( clientId );
const { getBlockName, getBlockAttributes } = select(
'core/block-editor'
);
return {
attributes: getBlockAttributes( clientId ),
name: getBlockName( clientId ),
};
},
[ clientId ]
);
Expand All @@ -40,5 +53,13 @@ export default function BlockTitle( { clientId } ) {
return null;
}

return blockType.title;
const { title } = blockType;
const label = getBlockLabel( blockType, attributes );

// Label will often fall back to the title if no label is defined for the
// current label context. We do not want "Paragraph: Paragraph".
if ( label !== title ) {
return `${ title }: ${ truncate( label, { length: 15 } ) }`;
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 know if this was discussed but are we certain that "truncate" works well with internationalized strings? Should we rely on CSS truncate instead?

Copy link
Contributor

@Copons Copons Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's aware of unicode chars, and it seems to handle them well in my (naïve) tests.

I've tried truncating a string with Japanese kanji and one with "complex" emoji, and in both cases the truncation happened as expected, with kanji and emojis being treated as single characters.

truncate( 'lorem ipsum dolor sit amet', { length: 10 } );
// "lorem i..."

truncate( 'kanji 僕は猫です', { length: 10 } );
// "kanji 僕..."

truncate( 'emoji 👩‍🍳👡👩‍👩‍👦‍👦🐏🐳🤳🏽🕵🏿‍♀️', { length: 10 } );
// "emoji 👩‍🍳..."

Note: that emoji string would be interpreted as this:
Screenshot 2020-07-16 at 12 57 53

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more thinking about length. Different languages have different sizes for characters, so I was wondering if "pixels" is better than "character length" in these situations.

To clarify I don't have experience with this personally, so just wondering if there's precedent. Maybe @swissspidy knows.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm good question! I think I have to defer to @naokomc to get her view point as a Japanese speaker & locale manager to know whether truncating like this would work in that language.

I know that for word counting we have to differentiate between counting words and characters depending on the locale, but for truncating I am not sure..

Copy link

Choose a reason for hiding this comment

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

Quoting Text size in translation:

Chinese, Japanese and Korean, amongst others, are scripts that typically have more complicated characters than those in the Latin script.

I tried to search for the widest Japanese character but couldn't find the answer. AFAIK there isn't a super-wide character in Japanese, they are more or less the same (although, different characters have different kerning if you are using non-proportional font).

Most Japanese character width itself is similar to uppercase "W", which is the widest character in the alphabet.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like this truncate method should work ok then?

}
return title;
}
64 changes: 61 additions & 3 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 @@ -22,6 +22,24 @@ jest.mock( '@wordpress/blocks', () => {

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

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

case 'name-with-long-label':
return { title: 'Block With Long Label' };
}
},
__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.';

default:
return title;
}
},
};
Expand All @@ -34,14 +52,22 @@ jest.mock( '@wordpress/data/src/components/use-select', () => {
} );

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

const wrapper = shallow( <BlockTitle /> );

expect( wrapper.type() ).toBe( null );
} );

it( 'renders nothing if block type does not exist', () => {
useSelect.mockImplementation( () => 'name-not-exists' );
useSelect.mockImplementation( () => ( {
name: 'name-not-exists',
attributes: null,
} ) );
const wrapper = shallow(
<BlockTitle clientId="afd1cb17-2c08-4e7a-91be-007ba7ddc3a1" />
);
Expand All @@ -50,11 +76,43 @@ describe( 'BlockTitle', () => {
} );

it( 'renders title if block type exists', () => {
useSelect.mockImplementation( () => 'name-exists' );
useSelect.mockImplementation( () => ( {
name: 'name-exists',
attributes: null,
} ) );

const wrapper = shallow(
<BlockTitle clientId="afd1cb17-2c08-4e7a-91be-007ba7ddc3a1" />
);

expect( wrapper.text() ).toBe( 'Block Title' );
} );

it( 'renders label if it is set', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-label',
attributes: null,
} ) );

const wrapper = shallow(
<BlockTitle clientId="afd1cb17-2c08-4e7a-91be-007ba7ddc3a1" />
);

expect( wrapper.text() ).toBe( 'Block With Label: Test Label' );
} );

it( 'truncates the label if it is too long', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-long-label',
attributes: null,
} ) );

const wrapper = shallow(
<BlockTitle clientId="afd1cb17-2c08-4e7a-91be-007ba7ddc3a1" />
);

expect( wrapper.text() ).toBe(
'Block With Long Label: This is a lo...'
);
} );
} );