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.

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

embed: fix translation string #6354

Merged
merged 2 commits into from
Apr 30, 2018
Merged

embed: fix translation string #6354

merged 2 commits into from
Apr 30, 2018

Conversation

ajitbohra
Copy link
Member

Description

Fix translation string to use sprintf

How has this been tested?

  • Manually
  • npm run test:unit

Screenshots

N/A

Types of changes

Translation fix

Checklist:

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

@gziolo gziolo added this to the 2.8 milestone Apr 23, 2018
@aduth
Copy link
Member

aduth commented Apr 24, 2018

Thanks for the revision.

Would this string suffer from the issues described in #5361 ?

@mcsf
Copy link
Contributor

mcsf commented Apr 27, 2018

Would this string suffer from the issues described in #5361 ?

Yes. I brought this up in #6124 (comment).

In practice, I dare say this particular string would be less affected than others, mainly since the variable to be interpolated is a proper noun referring to more or less the same kind of thing — a Web service, typically with a English-sounding name —, meaning it's more likely that gender will be consistent and more likely that inflexion is not needed.

But I stress the word likely: one can probably make it work in most Romance languages and modern Germanic, but some other languages really do need the inflexion, and the rendered string will be wrong. A Finnish speaker described this kind of string as "Nokia language" (as Nokia, itself Finnish, couldn't properly solve i18n), since the ideal translation would be something like:

Liitä URLit YouTubesta upottaaksesi sisältö tähän lohkoon

Note the inflected form of YouTube. But, because of the inability of translation systems to mark inflexion needs in things like embed names, the cop-out in Finnish goes something like:

Paste URLs from this service to embed the content in this block: %s

or, a tad more gracefully, by constructing the unit “Youtube -service”

Liitä URLit YouTube -palvelusta upottaaksesi sisältö tähän lohkoon

Others are expected to be affected too: Estonian, Hungarian, Turkish, etc.

@mcsf
Copy link
Contributor

mcsf commented Apr 27, 2018

My comment above is a bit academic and not very actionable, so here's the takeaway:

  • Translators will probably make it work, but in some languages it becomes awkward.
  • I don't have a strong opinion on keeping or rejecting this particular string (it's not a high-stakes string in the UI).

@mcsf
Copy link
Contributor

mcsf commented Apr 27, 2018

@ajitbohra, could you rebase this branch? The purpose of this PR was to fix how the string was provided, not what the string should be, so I don't want to block this. :)

@ajitbohra
Copy link
Member Author

ajitbohra commented Apr 30, 2018

@aduth guess it would not suffer from #5361

@mcsf have rebased the branch and thanks for your insights on translation have been spending some time over the weekend digging into it :)

@mcsf
Copy link
Contributor

mcsf commented Apr 30, 2018

@ajitbohra, thanks for the rebase. One last request: could you add a translators note? See example:

// translators: %s: Type of block (i.e. Text, Image etc)
const blockLabel = sprintf( __( 'Block: %s' ), blockType.title );

In this case, it would be:

// translators: %s: Name of service (e.g. VideoPress, YouTube)

@ajitbohra
Copy link
Member Author

@mcsf translators note added

@mcsf mcsf merged commit 3e36770 into WordPress:master Apr 30, 2018
@mcsf
Copy link
Contributor

mcsf commented Apr 30, 2018

Thanks! Merged.

@ajitbohra ajitbohra deleted the update/6110 branch May 3, 2018 13:36
@ajitbohra ajitbohra restored the update/6110 branch May 11, 2018 09:41
@ajitbohra ajitbohra deleted the update/6110 branch May 18, 2018 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants