WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 days ago

#48244 new defect (bug)

script-loader.php Need to use _n() when more than one results are found

Reported by: tobifjellner Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.5
Component: Editor Keywords: good-first-bug has-patch
Focuses: javascript Cc:

Description

Location:
https://build.trac.wordpress.org/browser/trunk/wp-includes/script-loader.php?marks=1178#L1178

Content:

'manyResults'  => __( '%d results found. Use up and down arrow keys to navigate.' ),

should (if possible) be converted to use _n() in order for some languages to be able to correctly construct the phrase.

Attachments (9)

48244.patch (905 bytes) - added by shaharia.azam 22 months ago.
_n() has been applied in script-loader.php when more than one results are found
48244.2.patch (742 bytes) - added by shaharia.azam 22 months ago.
String is returning correctly. Removed .replace() from plugin.js
48244.3.patch (651 bytes) - added by shaharia.azam 22 months ago.
String is returning correctly. Removed .replace() from tags-suggest.js
48244.4.patch (1.8 KB) - added by shaharia.azam 22 months ago.
It's for 4.5 . Both plugin.js and script-loader.php has been updated
48244.5.diff (2.1 KB) - added by shaampk1 22 months ago.
Merged plugin.js, tags-suggest.js, and script-loader.php patches into one .diff file
48244.5.patch (1.6 KB) - added by shaharia.azam 22 months ago.
As per @swissspidy on slack, _n implemented on plugin.js & tags-suggest.js
48244.2021091100.patch (4.0 KB) - added by rsiddharth 7 days ago.
Initial version (2021091100).
48244.2021091300.patch (5.9 KB) - added by rsiddharth 5 days ago.
Patch version 2021091300 - addresses issues pointed out by @swissspidy
48244.2021091500.patch (6.0 KB) - added by rsiddharth 3 days ago.
Patch version 2021091500 - addresses issues pointed out by @swissspidy

Download all attachments as: .zip

Change History (22)

#1 @swissspidy
2 years ago

The correct number is only known client-side, so for correct results one would need to use _n() from the @wordpress/i18n JS package.

This would need to happen in at least two places:

https://github.com/WordPress/wordpress-develop/blob/3b14a06c8cb187d8abd3d4af8ea9c0ea3a475228/src/js/_enqueues/vendor/tinymce/plugins/wplink/plugin.js#L459-L469
https://github.com/WordPress/wordpress-develop/blob/715a65c56142262b938d6860f840e201b1fe074a/src/js/_enqueues/admin/tags-suggest.js#L135-L144

However, this is by far not the only place where plural forms in JS could or should be improved. In #20491 you can find a huge patch with possible changes.

#2 @SergeyBiryukov
2 years ago

  • Version changed from trunk to 4.5

Introduced in [36806].

#3 @shaampk1
23 months ago

Hey folks,

This ticket really got my attention. I found it an interesting issue to work with. I know there has been already said a lot about L10n which I have been reading and trying to catch up with for the past few days now but still, I am not sure how far I have come regarding this issue. I'd appreciate it if you could review the following code snippet and let me know what I am missing.

For script-loader.php

	// Strings for 'jquery-ui-autocomplete' live region messages
	did_action( 'init' ) && $scripts->localize(
		'jquery-ui-autocomplete',
		'uiAutocompleteL10n',
		array(
			'noResults'    => __( 'No results found.' ),
			/* translators: %s: Number of results found when using jQuery UI Autocomplete. */
			'manyResults' => _n( '%s result found. Use up and down arrow keys to navigate.', '%s results found. Use up and down arrow keys to navigate.', did_action( 'init' ) ),
			'itemSelected' => __( 'Item selected.' ),
		)
	);

For plugin.js & tags-suggest.js

messages: {
	noResults: ( typeof window.uiAutocompleteL10n !== 'undefined' ) ? window.uiAutocompleteL10n.noResults : '',
	results: function( number ) {
		if ( typeof window.uiAutocompleteL10n !== 'undefined' ) {
			if ( number > 1 ) {
				return window.uiAutocompleteL10n.manyResults.replace( '%s', number );
			}

			return window.uiAutocompleteL10n.oneResult;
		}
	}
}

Thank you!

Last edited 22 months ago by shaampk1 (previous) (diff)

@shaharia.azam
22 months ago

_n() has been applied in script-loader.php when more than one results are found

@shaharia.azam
22 months ago

String is returning correctly. Removed .replace() from plugin.js

@shaharia.azam
22 months ago

String is returning correctly. Removed .replace() from tags-suggest.js

This ticket was mentioned in Slack in #core by shaharia. View the logs.


22 months ago

@shaharia.azam
22 months ago

It's for 4.5 . Both plugin.js and script-loader.php has been updated

@shaampk1
22 months ago

Merged plugin.js, tags-suggest.js, and script-loader.php patches into one .diff file

@shaharia.azam
22 months ago

As per @swissspidy on slack, _n implemented on plugin.js & tags-suggest.js

#5 @rebasaurus
19 months ago

  • Keywords has-patch added; needs-patch removed

#6 @swissspidy
19 months ago

  • Keywords needs-patch added; has-patch removed

Please refer to the documentation on how to properly use these functions in JS: https://github.com/WordPress/gutenberg/tree/6942c1df30250d5f27e36c2938b7905acf79644c/packages/i18n#usage

We'd need to make sure that these scripts will have wp-i18n as a dependency as well.

Also: please test your patches.

#7 @johnbillion
19 months ago

  • Component changed from Script Loader to Editor
  • Focuses javascript added

@rsiddharth
7 days ago

Initial version (2021091100).

#8 @rsiddharth
7 days ago

  • Keywords has-patch added; needs-patch removed

@swissspidy @SergeyBiryukov uploaded initial version of the patch.

#9 @swissspidy
6 days ago

Thanks for the patch. Two things I noticed at first glance:

  • uiAutocompleteL10n definition needs to be removed in script-loader.php
  • window.uiAutocompleteL10n needs to be deprecated using deprecateL10nObject

See r48925 for an example of how that deprecation works.

@rsiddharth
5 days ago

Patch version 2021091300 - addresses issues pointed out by @swissspidy 

#10 @rsiddharth
5 days ago

@swissspidy, Thank you for reviewing the patch.

I've:

  • Removed uiAutocompleteL10n from script-loader.php
  • Deprecated window.uiAutocompleteL10n using deprecateL10nObject

Questions:

  • Is script-loader.php:1099 (in the patch) the right place to add wp-i18n?
    I see the wplink plugin used in wp_tinymce_inline_scripts. Wondering if
    wp-i18n needs to be loaded there somehow?

#11 @swissspidy
5 days ago

Nice! We're getting there!

With the deprecation being done in common.js, common should be added as a dependency to both scripts.

No need to add wp-i18n as a dependency.

Instead, use $scripts->set_translations( 'wplink' );, which will do this automatically. tags-suggest already has this call, so all good there.

Nice find with r36806! Since that changeset is referencing a ticket with milestone 4.5.0, that would be the correct version for the @since annotation.

#12 @SergeyBiryukov
3 days ago

  • Milestone changed from Awaiting Review to 5.9

@rsiddharth
3 days ago

Patch version 2021091500 - addresses issues pointed out by @swissspidy

#13 @rsiddharth
3 days ago

@swissspidy, uploaded new version of the patch with the following changes:

  • Added common as a dependency for wplink and tags-suggest.
  • Used $scripts->set_translations( 'wplink' ); for loading wp-i18n for wplink.
  • Changed @since for window.uiAutocompleteL10n to 4.5.0.
Note: See TracTickets for help on using tickets.