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

Missing text-domain for string translations #21

Closed
bahiirwa opened this issue Jan 3, 2020 · 5 comments
Closed

Missing text-domain for string translations #21

bahiirwa opened this issue Jan 3, 2020 · 5 comments
Assignees
Labels
Status: Completed This item has been implemented or otherwise addressed. Type: Enhancement This item is a feature request or enhancement to existing functionality.

Comments

@bahiirwa
Copy link

bahiirwa commented Jan 3, 2020

Description

When I run the pre-hooks for building my plugin, I throws a warning for these lines in
classes/class-wc-update-client.php specifically L182-184.

// This will make the jQuery below work with various languages.
$text1 = esc_html__('Compatible up to:');
$text2 = esc_html__('Reviews');
$text3 = esc_html__('Read all reviews');

Was this intentional to have esc_html__ with a missing text domain?

@johnalarcon
Copy link
Collaborator

johnalarcon commented Jan 3, 2020

Hi @bahiirwa, good eye! The text domain hasn't been forgotten; these are actually the core texts, so, they don't require a text domain. :)

@johnalarcon johnalarcon added the Status: Invalid This item is not reproducible, is not permitted, or is otherwise invalid. label Jan 3, 2020
@johnalarcon
Copy link
Collaborator

On further consideration, I'm re-opening this issue. I see no harm in simply adding the text domain. It won't break anything and, well, it's only 3 strings. I'll get this into the 1.0.0 final release, which I anticipate in another week or so, just to be sure no further issues roll in.

@johnalarcon johnalarcon reopened this Jan 3, 2020
@johnalarcon johnalarcon added Status: Accepted This item has been accepted and will be implemented or otherwise addressed. Type: Enhancement This item is a feature request or enhancement to existing functionality. and removed Status: Invalid This item is not reproducible, is not permitted, or is otherwise invalid. labels Jan 3, 2020
@xxsimoxx
Copy link
Owner

xxsimoxx commented Jan 3, 2020

I think you should consider it. If you add text domain to "core" texts, they will not be translated. Never, because strings in UpdateClient.class.php for being translated need to have my-text-domain as text domain (when used not for Update Manager itself).

The line that adds the "View details" link, where there is codepotent-update-manager as text domain (and a typo, it should be esc_html__ and not esc_html) for me creates inconsistent UX in the plugin page.

image

@johnalarcon
Copy link
Collaborator

johnalarcon commented Jan 3, 2020

The line that adds the "View details" link, where there is codepotent-update-manager as text domain (and a typo, it should be esc_html__ and not esc_html) for me creates inconsistent UX in the plugin page.

Thanks, @xxsimoxx, for the additional information. Please open a dedicated issue for this part as it is different than the issue initially raised. I just don't want it to become lost in the mix. 😄

@johnalarcon johnalarcon added Status: Completed This item has been implemented or otherwise addressed. and removed Status: Accepted This item has been accepted and will be implemented or otherwise addressed. labels Jan 8, 2020
@johnalarcon
Copy link
Collaborator

It seems I originally didn't comprehend what Simone was trying to say above. As he correctly points out, adding a text domain to the update client texts actually breaks the core translation functionality. In light of this, the text domain will be re-removed.

For those who have trouble with builds or sniffs, just add your own text domain to the client file – the same domain that you're using in your plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed This item has been implemented or otherwise addressed. Type: Enhancement This item is a feature request or enhancement to existing functionality.
Projects
None yet
Development

No branches or pull requests

3 participants