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

Today widget localization #15614

Merged

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented Jan 8, 2021

Fixes #15157

Note:
To achieve this, I copied the existing Today Widget Localizable.strings. Since this did not contain the "Today" string, I retrieved this from the main app, but since it was a manual process, translations will need to be verified. Also, we need to add translations of the placeholder/unconfigured view, that are not present at the moment.

To test:

  • Install Today Widget, medium size (small size would not contain all the strings)
  • make sure the strings look correct in the current device language
  • change the device language (as many times as you want) and make sure that the strings still look correct

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@Gio2018 Gio2018 added this to the 16.5 milestone Jan 8, 2021
@Gio2018 Gio2018 self-assigned this Jan 8, 2021
@Gio2018 Gio2018 changed the title Feature/15157 today widget localization Today widget localization Jan 8, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 8, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@Gio2018 Gio2018 modified the milestones: 16.5, 16.6 Jan 8, 2021
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Works as described!

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Hey @Gio2018!

Thanks for the ping about this. I discussed with @AliSoftware this morning and we have one suggestion – currently, the keys here share names with similar keys in the main app. Because widgets have different considerations (as a for-instance, we may need to use a very compact string in non-English languages in order for them to fit), we think it would be worth namespacing the keys in order to avoid collisions between strings in the main app. This would be an excellent case for using synthetic key names (such as TODAY_WIDGET_COMMENTS_LABEL – then providing context such as "this will appear in a widget so should be represented by a concise translation" in the translation notes).

In terms of updating these, for now this will likely be a manual process (ie – we'll ingest the widget base localization into GlotPress, but copying the translations back into the widget will have to happen by hand for now). We're working on automation for this, but it's unlikely to ship before this needs to land.

Hope that helps – happy to discuss further!!

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 12, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

…lue if localization is missing, TodayWidgetView
@AliSoftware
Copy link
Contributor

AliSoftware commented Jan 13, 2021

Hey @Gio2018 , sorry to come late to the party.

To complement what Jeremy said above, specifically on the use of synthetic key names:

  • I'm all for using synthetic keys in our .strings file, as I once explained in a previous P2, so the widget could be a good occasion to introduce that!
  • I'd prefer for us to use non-screaming, reverse-dns notation (with .) for those key name conventions tho, rather than full-caps and _. Mainly because it's a common convention in most projects out there, but also because SwiftGen supports this format out of the box, which would make our adoption of it easier if we come to it one day. So for example, I'd suggest using something like widget.today.comments.label rather than TODAY_WIDGET_COMMENTS_LABEL.
  • We might need to experiment with how the tooling react to this new convention of using synthetic keys. Some things in our current tooling assumes that the keys we use are the English copy – as that's what we've been using so far in the apps – and assume that the source of truth for the copies are the codebase (and extracted by genstrings) and generate the Base.lproj/Localizable.strings (or en.lproj's) from it; so we might want to ensure that when you write the English copies in the en.lproj/Base.lproj yourself it won't be overwritten with "key" = "key"; by our tooling accidentally. That's one more reason why I agree with Jeremy that we should keep those updates manual for now until we have some time to ensure our tooling is ready and update it accordingly

I'll let you re-update the keys with the new convention, and I'm hoping that for potential future changes in the copies we won't forget to do manual updates… be sure to keep us in the loop if that happens.

@diegoreymendez
Copy link
Contributor

I like the reverse DNS suggestion. The only question in my mind is if we need to namespace it further adding what app the widget belongs to.

wpios.widget.today.comments.label

I think this will depend on whether we're using our WPiOS GlotPress project for other Apps too (which I don't know), or just for WPiOS.

My 2 cents.

@AliSoftware
Copy link
Contributor

Good thinking; but we do use separate GlotPress projects for the other apps, so it's unnecessary to namespace the key with the app name 🙂


For your info: we do use a single GP project for all targets of a given app (main app target, widgets, libs and dependencies…) and merge all of the .strings files of an app (like WordPress-iOS) and its dependencies in a dedicated single .strings file when we prepare the strings for import by GlotPress, so it imports them all in a single GP project associated with that app/product.

That way translators of a given app don't need to go into different GP projects to translate various parts of the app (and don't need to know about the technical details of which lib/target the string comes from) and only deal with a single GP project for a given app. This is alsy why we want to namespace the key names by target (widget vs app), to avoid conflicts when we merge strings from different targets in the unique "WordPress iOS" GlotPress project.

But that merging is only per-product / per-app. Each apps still have its own, separated GlotPress project, so there's not risk a key in WordPress-iOS could conflict with WooCommerce-iOS for example.

@Gio2018
Copy link
Contributor Author

Gio2018 commented Jan 13, 2021

Thanks @jkmassel @AliSoftware @diegoreymendez for the reviews. I updated the keys and added the unconfigured view title (which will need to be translated).

@Gio2018 Gio2018 requested a review from jkmassel January 13, 2021 15:58
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

I tested this using en-CA and fr as the languages. In both cases because it was already translated, the strings showed up as expected.

To test the default case, I deleted the en-CA and fr localizations for the widget and re-ran the app. The strings appeared correctly using the default values.

I took a look at the LocalizedStringKey extension and it's nice and straightforward. I've left a couple of nitpicks for your consideration, but they're not required!

extension LocalizedStringKey {
static let defaultBundle = Bundle(for: HomeWidgetTodayRemoteService.self)

/// TODO - TODAYWIDGET - LocalizedStringKey is used as a wrapper of NSLocalizedString, in order to use synthetic keys and assign a default value
Copy link
Contributor

Choose a reason for hiding this comment

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

It might not be worth mentioning this as a TODO just because IMHO it's unlikely to change any time soon. It seems like this was a great (missed) opportunity for Apple to clean this up and have LocalizedStringKey have proper fallback behaviour and they didn't, so I wouldn't expect this to change.

I like the comment though explaining why it exists and the circumstances under which it could be replaced. Just not sure if TODO is that useful since it might be in that state for a looooooong time 😄

@@ -0,0 +1,12 @@
import SwiftUI

Copy link
Contributor

Choose a reason for hiding this comment

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

Is two spaces between import statements and the extension definition typical?

@@ -0,0 +1,17 @@
/* Title of comments label in today widget */
"widget.today.comments.label" = "Comments";
Copy link
Contributor

Choose a reason for hiding this comment

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

The reverse-DNS notation is much better than the ALL CAPS for sure!!

@Gio2018 Gio2018 merged commit 913a40a into feature/ios-14-today-widget Jan 13, 2021
@Gio2018 Gio2018 deleted the feature/15157-today-widget-localization branch January 13, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants