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

Improve translatable strings containing "%s" to have a translator comment #7201

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jun 7, 2018

When we use %s in a string and it is not totally obvious what %s means when reading the rest of the string, we should provide a translator comment.

Tries to address: #5361

How has this been tested?

Only comments were added to the translations was added in this changes, some smoke testing, and the automated tests should be enough to catch problems.

@jorgefilipecosta jorgefilipecosta self-assigned this Jun 7, 2018

@jorgefilipecosta jorgefilipecosta requested a review from afercia Jun 7, 2018

@ocean90

The use of _x() is incorrect here. The context function is used to distinguish the same string used in different contexts. For additional info, like explaining placeholders, a translators comment should be added.

@jorgefilipecosta jorgefilipecosta changed the title from Improve translatable strings containing "%s" to have a context to Improve translatable strings containing "%s" to have a translator comment Jun 7, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 7, 2018

Thank you for explanation @ocean90, the code was updated to use translator comments.

@@ -80,6 +80,7 @@ export class ServerSideRender extends Component {
<Placeholder><Spinner /></Placeholder>
);
} else if ( response.error ) {
// translators: %s: error message describing the problem
return (
<Placeholder>{ sprintf( __( 'Error loading block: %s' ), response.errorMsg ) }</Placeholder>

This comment has been minimized.

@afercia

afercia Jun 7, 2018

Contributor

Not sure if in JS it's the same, but ideally the translators comment should be immediately before the line where it is used e..g.:

<Placeholder>{ sprintf( 
    // translators: %s: error message describing the problem
    __( 'Error loading block: %s' )

@ocean90 ?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 18, 2018

Member

Hi @afercia thank you for referring this, the code was updated now translator comments are immediately before the line where they are used.

@jorgefilipecosta jorgefilipecosta added this to the 3.1 milestone Jun 20, 2018

@gziolo

gziolo approved these changes Jun 21, 2018

LGTM 👍
There is one thing where we might need to move the comment closer to the translation function.

@@ -34,6 +34,7 @@ export default function editorMediaUpload( {
const errorHandler = ( { file, sizeAboveLimit, generalError } ) => {
let errorMsg;
if ( sizeAboveLimit ) {
// translators: %s: file name

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

You probably need to move it inside sprintf just before the line with __( ... ).

There is no usage of _x() anymore so dismissing this one to unblock PR. @ocean90 feel free to review again.

@jorgefilipecosta jorgefilipecosta merged commit 859f016 into master Jun 21, 2018

2 checks passed

codecov/project 46.89% (-0.01%) compared to 86011f1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/improve-translatable-strings branch Jun 21, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 21, 2018

Thank you for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment