Better description for embed blocks by ajitbohra · Pull Request #6124 · WordPress/gutenberg

ajitbohra · 2018-04-11T18:13:43Z

Description

Fixes #6110
Generic description for embed blocks replaced with a personalized message by using embed title/aliases as suggested by @jasmussen

This embed block embeds content from ${ title }

How Has This Been Tested?

Manually adding embed blocks and inspecting description in inspector/sidebar
npm run test-unit

Screenshots (jpeg or gifs if applicable):

N/A

Types of changes

Non-breaking change

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

jasmussen · 2018-04-12T07:19:02Z

Nice, thank you for working on this!

The change seem to be good for all the embed aliases:

Though perhaps we could tune the description slightly so that it isn't as redundant. Maybe something like:

Paste URLs from ${ title } to embed the content in this block.

However this change doesn't look too good with the generic embed block:

Can we make an exception for the generic embed (the one you get if you just insert "Embed") block, so it keeps its original description?

ajitbohra · 2018-04-12T15:21:23Z

@jasmussen Description updated

Not sure if below approach is right

description: 'Embed' !== title ?
			__( `Paste URLs from ${ title } to embed the content in this block` ) :
			__( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),

should we check for 'Embed' or __( 'Embed' ) ?

Screenshots:
Generic Embed

Twitter Embed

jasmussen · 2018-04-13T07:32:41Z

This looks great to me, thank you! I've added a review note for a quick glance at the code. Thanks again!

gziolo · 2018-04-13T07:35:53Z

blocks/library/embed/index.js

-
-		description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),
-
+		description: 'Embed' !== title ?


I would rather pass block's name and perform this check based on it.

@gziolo will give that a try

Cool, thanks 👍

I have another idea which is simpler: You can pass description for the base Embed block and compose it based on title for all other occurences:

export const settings = getEmbedBlockSettings( { title: __( 'Embed' ), description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ), ...

and inside the getEmbedBlockSettings function:

description: description || __( `Paste URLs from ${ title } to embed the content in this block` ),

@gziolo this looks good compared to title or name safe & fewer code changes :)

have updated the PR

gziolo · 2018-04-18T06:52:04Z

blocks/library/embed/index.js

-
-		description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),
-
+		description: description || __( `Paste URLs from ${ title } to embed the content in this block` ),


I think this sentence should end with a period for consistency.

gziolo

I added the missing period. This looks great. Thanks for submitting this fix 👍

jasmussen · 2018-04-18T07:35:34Z

🎉

mcsf · 2018-04-18T16:15:39Z

Note that this is a tricky change when it comes to i18n. In English, there is no inflexion in "Service" in the sentence "Paste URLs from Service to embed the content", but other languages might have them. As a general rule, it's very hard to respect i18n when generating strings of this kind, with names interpolated.

A solution, although very cumbersome, would be to add strings for all these services in specific grammar cases, e.g.:

the embed type's name would be _x( 'YouTube', 'nominative' )
the fragment in its description would be _x( 'YouTube', 'ablative' )

But I don't think the above would be practical at all, and would still likely break for some odd language that operates differently.

(cf ablative)

aduth · 2018-04-18T19:35:32Z

blocks/library/embed/index.js

-
-		description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),
-
+		description: description || __( `Paste URLs from ${ title } to embed the content in this block.` ),


Because of how string extraction works, we cannot do variable string interpolation for translated strings.

This is the same as it is in PHP:

https://codex.wordpress.org/I18n_for_WordPress_Developers#Placeholders

We should use a placeholder here, and wp.i18n.sprintf to do the variable replacement.

@aduth my bad missed that part have updated the same here #6354

* Better description for embed blocksn * update description * Check based on description * Blocks: Add a period to the description of embed

Better description for embed blocksn

bfeef3d

update description

bf56f13

jasmussen requested a review from a team April 13, 2018 07:32

gziolo reviewed Apr 13, 2018

View reviewed changes

Check based on description

909cb38

gziolo reviewed Apr 18, 2018

View reviewed changes

Blocks: Add a period to the description of embed

f3b3f92

gziolo added [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement. labels Apr 18, 2018

gziolo approved these changes Apr 18, 2018

View reviewed changes

gziolo merged commit 4578daf into WordPress:master Apr 18, 2018

gziolo added this to the 2.7 milestone Apr 18, 2018

aduth reviewed Apr 18, 2018

View reviewed changes

ajitbohra deleted the fix/6110 branch April 23, 2018 12:20

nuzzio pushed a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Better description for embed blocks (WordPress#6124)

7a51d7a

* Better description for embed blocksn * update description * Check based on description * Blocks: Add a period to the description of embed

mcsf mentioned this pull request Apr 27, 2018

embed: fix translation string #6354

Merged

4 tasks

mcsf mentioned this pull request Feb 8, 2019

Update text displayed when an embed can't be previewed #13715

Merged

5 tasks


		description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),

		description: 'Embed' !== title ?


		description: __( 'The Embed block allows you to easily add videos, images, tweets, audio, and other content to your post or page.' ),

		description: description \|\| __( `Paste URLs from ${ title } to embed the content in this block` ),

Conversation

ajitbohra commented Apr 11, 2018

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Uh oh!

jasmussen commented Apr 12, 2018

Uh oh!

ajitbohra commented Apr 12, 2018

Uh oh!

jasmussen commented Apr 13, 2018

Uh oh!

gziolo Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

ajitbohra Apr 14, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajitbohra Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Apr 18, 2018

Uh oh!

mcsf commented Apr 18, 2018

Uh oh!

aduth Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

ajitbohra Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gziolo Apr 16, 2018 •

edited

Loading