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

Add context-splitting for the string "none" #22095

Closed

Conversation

audrasjb
Copy link
Contributor

@audrasjb audrasjb commented May 5, 2020

Description

The strings "None" and "none" are used in many places.
Unfortunately, this string is impossible to translate correctly in many languages.
The reason is that depending on what noun is described, this word may need to be presented in various forms.
For reference, see the original ticket on Trac.

This changeset replaces __( 'None' ) occurrences with _x( 'None', 'Some context )

Fixes #22094

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@audrasjb
Copy link
Contributor Author

audrasjb commented May 5, 2020

Fixes #22094

@audrasjb
Copy link
Contributor Author

audrasjb commented May 7, 2020

I fixed one of the 3 linting errors, but I can't see what I need to do to fix the two last ones. Could someone explain me the issue please? thanks 🙂

@audrasjb
Copy link
Contributor Author

audrasjb commented May 8, 2020

Ok checks are green now 🍏 🙂

@youknowriad
Copy link
Contributor

youknowriad commented May 12, 2020

Seems good but I don't have a good i18n knowledge to judge the context splitting here. ccc @swissspidy @mcsf

@@ -343,7 +343,7 @@ class ButtonEdit extends Component {
icon={ ! isCompatibleWithSettings && LinkRelIcon }
label={ __( 'Link Rel' ) }
value={ rel || '' }
valuePlaceholder={ __( 'None' ) }
valuePlaceholder={ _x( 'None', 'Rel attribute value' ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
valuePlaceholder={ _x( 'None', 'Rel attribute value' ) }
valuePlaceholder={ _x( 'None', 'Link rel attribute value placeholder' ) }

@swissspidy
Copy link
Member

One suggestion and 1 conflict that needs to be resolved, otherwise LGTM

{ value: 'none', label: __( 'None' ) },
{
value: 'none',
label: _x( 'None', 'Preload value' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

Suggested change
label: _x( 'None', 'Preload value' ),
label: _x( 'None', '"Preload" value' ),

would make it more explicit.

I was also going to suggest appending "for Audio block" but I see that Video is meant to reuse this context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which, that __( 'Preload' ) just a few lines above seems like a quick win to add to this PR. 😉

_x( 'Preload', 'noun; Audio block parameter' )

@youknowriad
Copy link
Contributor

Looks like this is approved and went a bit stale? Can we refresh nd land it?

@pinarol pinarol removed their request for review October 1, 2020 14:36
Base automatically changed from master to trunk March 1, 2021 15:43
@aristath
Copy link
Member

I merged trunk and resolved conflicts in trunk...fix/contextualize-none-strings
@audrasjb you can merge that branch in your fork if you want to update it (audrasjb/gutenberg@fix/contextualize-none-strings...WordPress:fix/contextualize-none-strings), this one would be nice to land 👍 🚢

@ramonjd ramonjd added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Aug 26, 2021
@ramonjd
Copy link
Member

ramonjd commented Aug 26, 2021

@audrasjb I'm happy to carry this one forward if you don't have time. Agree with other folks that it would be good to land to make life for our translators a little easier 😄

@audrasjb
Copy link
Contributor Author

Thank you for the proposal @ramonjd. Very appreciated cause I'm currently on holidays 😅

@ramonjd
Copy link
Member

ramonjd commented Aug 26, 2021

Thank you for the proposal @ramonjd. Very appreciated cause I'm currently on holidays 😅

Nice! Hope you have a great time. Wish I were on holidays too 🤣

I'll spin up a new PR soon. Thanks for your work on this.

ramonjd added a commit that referenced this pull request Aug 27, 2021
…rom #22095

This is because "None" can be translated differently in languages other than English depending on the context.
@ramonjd
Copy link
Member

ramonjd commented Aug 27, 2021

Updated PR #34341

Closing this one.

Thank you!

@ramonjd ramonjd closed this Aug 27, 2021
ramonjd added a commit that referenced this pull request Aug 27, 2021
* Adding context to 'none' strings, and also implementing suggestions from #22095
This is because "None" can be translated differently in languages other than English depending on the context.

* As always, my linter was asleep
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context-splitting needed for the string "none"
6 participants