WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#37290 closed defect (bug) (fixed)

Shiny Updates: Different strings for plugins and themes?

Reported by: pavelevap Owned by: 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 3 years ago.
37290.diff (10.1 KB) - added by swissspidy 3 years ago.
37290.2.diff (9.8 KB) - added by swissspidy 3 years ago.
37290.3.diff (10.1 KB) - added by swissspidy 3 years ago.
37290.4.diff (14.8 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (37)

#1 @swissspidy
3 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
3 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
3 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
3 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 3 years ago by SergeyBiryukov (previous) (diff)

#5 @swissspidy
3 years ago

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

#6 in reply to: ↑ 4 @swissspidy
3 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
3 years ago

#7 @swissspidy
3 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
3 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 3 years ago by SergeyBiryukov (previous) (diff)

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


3 years ago

#10 @afercia
3 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 3 years ago by afercia (previous) (diff)

#11 @afercia
3 years ago

  • Focuses accessibility added

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

@swissspidy
3 years ago

#12 @swissspidy
3 years ago

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

@swissspidy
3 years ago

#13 @swissspidy
3 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
3 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 3 years ago by afercia (previous) (diff)

#15 @pavelevap
3 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
3 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
3 years ago

#17 in reply to: ↑ 16 @afercia
3 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
3 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.


3 years ago

#20 @swissspidy
3 years ago

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

#21 @ocean90
3 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
3 years ago

In 38070:

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

See #37290.

#23 @SergeyBiryukov
3 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
3 years ago

In 38081:

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

See #37290.

#25 @pavelevap
3 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
3 years ago

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

#27 @pavelevap
3 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
3 years ago

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

#29 @pavelevap
3 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
3 years ago

Apparently, yes, @afercia might know more :)

#31 @SergeyBiryukov
2 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
2 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.