Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#37290 closed defect (bug) (fixed)

Shiny Updates: Different strings for plugins and themes?

Reported by: pavelevap's profile pavelevap Owned by: ocean90's profile ocean90
Milestone: 4.6 Priority: high
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: has-patch
Focuses: accessibility Cc:

Description

There is only one string "%s installed!" for themes and plugins.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/script-loader.php?rev=37940#L634

But for example in Czech, we have to use different declension for themes "Twenty Sixteen 1.2 instalována!" and plugins "Hello Dolly 1.0 instalován!". It is the same example as Post and Page differences (which have its own strings). I can try to translate it differently, for example "Instalováno: Twenty Sixteen 1.2", but I am not sure about other languages.

Attachments (5)

37290.patch (2.7 KB) - added by SergeyBiryukov 10 years ago.
37290.diff (10.1 KB) - added by swissspidy 10 years ago.
37290.2.diff (9.8 KB) - added by swissspidy 10 years ago.
37290.3.diff (10.1 KB) - added by swissspidy 10 years ago.
37290.4.diff (14.8 KB) - added by swissspidy 10 years ago.

Download all attachments as: .zip

Change History (37)

#1 @swissspidy
10 years ago

  • Component changed from I18N to Upgrade/Install

There are currently 3 strings that are used for both plugins and themes:

  • Installing %s...
  • %s installed!
  • %s installation failed

In all cases, %s stands for Plugin/Theme name and version. To fix the problem with different declensions, we'd need to split these up into 6 different strings and add different comments/contexts for each. If that's not possible, the alternative translation you suggest is probably the best approach.

#2 @SergeyBiryukov
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.6

We have a different context for plugins and themes, see #27889.

We need to add a context for these new strings as well.

#3 @SergeyBiryukov
10 years ago

37290.patch is what I had in mind, but I could not find where installingLabel and installedLabel are used. The only instance I've found is in script-loader.php.

#4 follow-up: @SergeyBiryukov
10 years ago

%s was successfully deleted needs a context as well. In Russian, Czech, and other Slavic languages, "plugin" is masculine and "theme" is feminine.

P.S. If the string is only used for plugins, the translator comment should be updated. Currently it says "%s: Plugin or Theme name".

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#5 @swissspidy
10 years ago

  • Owner set to swissspidy
  • Priority changed from normal to high
  • Status changed from new to assigned

#6 in reply to: ↑ 4 @swissspidy
10 years ago

Replying to SergeyBiryukov:

%s was successfully deleted needs a context as well. In Russian, Czech, and other Slavic languages, "plugin" is masculine and "theme" is feminine.

P.S. If the string is only used for plugins, the translator comment should be updated. Currently it says "%s: Plugin or Theme name".

Good point. The string is part of the tmpl-item-deleted-row JS template in wp_print_update_row_templates(), which is used for the plugins list table and the network themes list table. This means we need to split the template up into 2 separate ones.

In the future, with i18n via JS, this won't be needed anymore.

@swissspidy
10 years ago

#7 @swissspidy
10 years ago

  • Focuses accessibility added
  • Keywords has-patch needs-testing added; needs-patch removed

In 37290.diff:

  • Add context to all the strings in question.
  • Uses installingLabel and installedLabel for the aria-label attributes. Seems like a bug that these weren't used before.
  • Fixes a spelling mistake in unknownError (An unknown error occurred).
  • Updates the labels in the fixtures file as well.

Adding the a11y label because of the aria-label changes.

#8 @SergeyBiryukov
10 years ago

Nitpicking:

'activatePlugin'             => is_network_admin() ? __( 'Network Activate' ) : __( 'Activate' ), 
'activateTheme'              => is_network_admin() ? __( 'Network Enable' ) : __( 'Activate' ), 

These strings are fine without a context. They are used without a context elsewhere, so adding a context in this single instance would be inconsistent and would just duplicate the string for no reason.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


10 years ago

#10 @afercia
10 years ago

  • Focuses accessibility removed

Definitely something that should be addressed, however going to remove the accessibility focus since it's not strictly related to accessibility.

Last edited 10 years ago by afercia (previous) (diff)

#11 @afercia
10 years ago

  • Focuses accessibility added

Sorry, not so fast :) Just noticed the changes to aria-label :)

@swissspidy
10 years ago

#12 @swissspidy
10 years ago

37290.2.diff removes the context from the activatePlugin and activateTheme strings again.

@swissspidy
10 years ago

#13 @swissspidy
10 years ago

37290.3.diff makes the patch apply cleanly against trunk again.

@SergeyBiryukov: would love your i18n input on that.
@afercia: a11y feedback welcome as well. Most notably, aria-label is now updated for each step of the plugin install process, i.e. "Install xy" -> "xy installed" -> "Activate xy". Previously, only the button text changed ("Install" -> "Installed" -> "Activate")

#14 @afercia
10 years ago

Looks good to me :) The only thing I've noticed so far is a small inconsistency:

Plugins:
when the link changes in "Activate" the aria label changes in "Activate {plugin-name}"

Themes:
when the link changes in "Activate" the aria label changes in "{theme-name} installed!"

Since there's already a wp.speak.a11y() message to confirm a successful installation ("Installation completed successfully.") maybe I'd consider to change the Themes aria-label using a text to announce the available action: "Activate {theme-name}".

Last edited 10 years ago by afercia (previous) (diff)

#15 @pavelevap
10 years ago

Should we add some more comments for translators that some of these strings are for accessibility? See #29748, it could be helpful in this case, I guess.

#16 follow-up: @swissspidy
10 years ago

@afercia I added these in 37290.4.diff. Does that work? Theoretically we could add those attributes to other links as well, but I didn't want to go down that rabbit hole…

@pavelevap That's probably better handled in #29748. We still haven't agreed on the exact wording IIRC.

@swissspidy
10 years ago

#17 in reply to: ↑ 16 @afercia
10 years ago

Replying to swissspidy:

@afercia I added these in 37290.4.diff. Does that work?

@swissspidy looks good to me! Maybe just check a translators comment /* translators: %s: Theme name */ in script-loader.php that seems a leftover.

Theoretically we could add those attributes to other links as well, but I didn't want to go down that rabbit hole…

Maybe better to wait for i18n via JS? Otherwise I'm afraid the amount of code necessary would grow insanely.

#18 @swissspidy
10 years ago

  • Keywords needs-testing removed

Thanks for pointing this out. Indeed a leftover. Also just noticed a wrong indentation in wp-admin/includes/update.php. We could fix these before commit.

Agree that it makes sense to wait for i18n via JS in case of the other strings.

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


10 years ago

#20 @swissspidy
10 years ago

  • Owner changed from swissspidy to ocean90
  • Status changed from assigned to reviewing

#21 @ocean90
10 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 38057:

Update/Install: Give context to some install/update strings to allow for differentiation between theme and plugin translations.

Props swissspidy, SergeyBiryukov.
Fixes #37290.

#22 @SergeyBiryukov
10 years ago

In 38070:

I18N: Remove a stray translator comment added in [38057].

See #37290.

#23 @SergeyBiryukov
10 years ago

In 38071:

I18N: After [38057], consistently use a context for other instances of Activate %s, Network Activate %s, and Delete %s strings.

See #37290.

#24 @SergeyBiryukov
10 years ago

In 38081:

Text Changes: Change Network deactivate %s to upper case, for consistency with Network Activate %s.

See #37290.

#25 @pavelevap
10 years ago

I noticed the same problem for strings related to updating :-( It was not problem before, because it worked only for plugins, but now it is wrong for themes. Should I reopen or create new ticket?

#26 @ocean90
10 years ago

@pavelevap Which strings need to be changed? Can you work on a patch?

#27 @pavelevap
10 years ago

Same strings as for installation, probably those 3:

Updating %s...
%s updated!
%s update failed

It is probably the same problem for other languages? @SergeyBiryukov

Not sure about the patch, probably also templates will have to change a little bit. I can look into it tomorrow...

#28 @SergeyBiryukov
10 years ago

@pavelevap: I've double-checked, and these strings are only used for plugins, not for themes:

#29 @pavelevap
10 years ago

Ahh, you are right! So updating themes is without aria-label strings? We have those for installing themes and plugins, updating plugins and only updating themes are missing them?

#30 @SergeyBiryukov
10 years ago

Apparently, yes, @afercia might know more :)

#31 @SergeyBiryukov
9 years ago

In 40034:

Update/Install: Give context to some more install/update strings to allow for differentiation between theme and plugin translations.

Fixes #39747. See #37290.

#32 @SergeyBiryukov
9 years ago

In 40035:

Update/Install: Give context to "Deleted! string to allow for differentiation between theme and plugin translations.

Props swissspidy.
See #39747. See #37290.

Note: See TracTickets for help on using tickets.