Opened 4 years ago
Last modified 3 months ago
#48244 reviewing defect (bug)
script-loader.php Need to use _n() when more than one results are found
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 4.5 |
Component: | Editor | Keywords: | has-patch needs-testing |
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 (10)
Change History (42)
#3
@
4 years 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!
This ticket was mentioned in Slack in #core by shaharia. View the logs.
4 years ago
#6
@
3 years 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.
#8
@
21 months ago
- Keywords has-patch added; needs-patch removed
@swissspidy @SergeyBiryukov uploaded initial version of the patch.
#9
@
21 months ago
Thanks for the patch. Two things I noticed at first glance:
uiAutocompleteL10n
definition needs to be removed inscript-loader.php
window.uiAutocompleteL10n
needs to be deprecated usingdeprecateL10nObject
See r48925 for an example of how that deprecation works.
#10
@
21 months ago
@swissspidy, Thank you for reviewing the patch.
I've:
- Removed
uiAutocompleteL10n
from script-loader.php - Deprecated
window.uiAutocompleteL10n
usingdeprecateL10nObject
Questions:
- Is script-loader.php:1099 (in the patch) the right place to add
wp-i18n
?
I see thewplink
plugin used inwp_tinymce_inline_scripts
. Wondering if
wp-i18n
needs to be loaded there somehow?
- Are the
@since
and@deprecated
version numbers correct for
window.uiAutocompleteL10n?
I guessed@since
from https://core.trac.wordpress.org/changeset/36806
#11
@
21 months 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.
#13
@
21 months ago
@swissspidy, uploaded new version of the patch with the following changes:
- Added
common
as a dependency forwplink
andtags-suggest
. - Used
$scripts->set_translations( 'wplink' );
for loadingwp-i18n
forwplink
. - Changed
@since
forwindow.uiAutocompleteL10n
to 4.5.0.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
20 months ago
#15
@
19 months ago
- Milestone changed from 5.9 to 6.0
With 5.9 Beta 1 is happening in less than 4 hours, moving this ticket to 6.0.
This ticket was mentioned in Slack in #core by rsiddharth. View the logs.
16 months ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
14 months ago
This ticket was mentioned in PR #2618 on WordPress/wordpress-develop by peterwilsoncc.
14 months ago
#18
https://core.trac.wordpress.org/ticket/48244
Existing patch, some cs fixes.
This ticket was mentioned in Slack in #core by sergey. View the logs.
14 months ago
#20
@
14 months ago
- Milestone changed from 6.0 to 6.1
As we are past soft-string freeze and now approaching RC1, I'm moving this ticket to the 6.1 milestone for remaining work to be carried out in the next cycle.
#21
@
11 months ago
I refreshed the existing PR and updated the @since
mentions.
Tests are still passing.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
9 months ago
#23
@
9 months ago
- Keywords needs-testing added; good-first-bug removed
@tobifjellner thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are updating the ticket with the following changes:
- Add keywords needs-testing since it needs to be tested. Even though JB mentioned in his most recent comment that the tests are still passing.
- Removing keywords good-first-bug since the ticket has multiple patches
Cheers!
Props to @audrasjb & @hilayt24
This ticket was mentioned in Slack in #core by chaion07. View the logs.
9 months ago
#25
@
9 months ago
We discussed this ticket during another recent bug-scrub session and would love for a response from JB on this ticket. Thanks!
Props to @mukesh27
#26
@
8 months ago
- Milestone changed from 6.1 to 6.2
With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as the proposed patch still needs testing.
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.