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.

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

Better description for embed blocks #6124

Merged
merged 4 commits into from Apr 18, 2018

Conversation

Projects
None yet
5 participants
@ajitbohra
Member

ajitbohra commented Apr 11, 2018

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

This comment has been minimized.

Contributor

jasmussen commented Apr 12, 2018

Nice, thank you for working on this!

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

screen shot 2018-04-12 at 09 15 39

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:

screen shot 2018-04-12 at 09 15 56

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?

screen shot 2018-04-12 at 09 18 24

@ajitbohra

This comment has been minimized.

Member

ajitbohra commented Apr 12, 2018

@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
genric

Twitter Embed
twitter

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Apr 13, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 13, 2018

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

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

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

This comment has been minimized.

@ajitbohra

ajitbohra Apr 14, 2018

Member

@gziolo will give that a try

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

Cool, thanks 👍

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

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` ),

This comment has been minimized.

@ajitbohra

ajitbohra Apr 18, 2018

Member

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

have updated the PR

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` ),

This comment has been minimized.

@gziolo

gziolo Apr 18, 2018

Member

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

@gziolo

gziolo approved these changes Apr 18, 2018

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

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

1 of 2 checks passed

codecov/project 43.86% (-0.69%) compared to 1325bae
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo added this to the 2.7 milestone Apr 18, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 18, 2018

🎉

@mcsf

This comment has been minimized.

Contributor

mcsf commented Apr 18, 2018

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)

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.` ),

This comment has been minimized.

@aduth

aduth Apr 18, 2018

Member

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.

This comment has been minimized.

@ajitbohra

ajitbohra Apr 23, 2018

Member

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

@ajitbohra ajitbohra deleted the ajitbohra:fix/6110 branch Apr 23, 2018

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

Better description for embed blocks (WordPress#6124)
* Better description for embed blocksn

* update description

* Check based on description

* Blocks: Add a period to the description of embed

@mcsf mcsf referenced this pull request Apr 27, 2018

Merged

embed: fix translation string #6354

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment