🌐✨First pass at internationalizing amp-story #14051

newmuis · 2018-03-16T17:49:18Z

This PR adds a simple service that swaps strings out depending on the lang attribute of the nearest ancestor. This service is currently in the amp-story extension, but could, in theory, be moved to src/.

The big problem with this is that it builds all strings for all languages into the binary that uses them. This won't scale to a large number of strings or languages. Ideally (I think?), we would have a way to build separate bundles based on the languages used, which we could dynamically fetch. That becomes an interesting UX problem, since there could be a delay before we have the strings.

Fixes #13556.

Loosely based on https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Internationalization

newmuis · 2018-03-16T18:08:02Z

/cc @calebcordry

gmajoulet · 2018-03-16T18:54:15Z

extensions/amp-story/0.1/messages.js

+   *     document.  If unspecified, will use the document-level language, if
+   *     one exists, or the default otherwise.
+   */
+  getMessage(messageId, opt_elementToUse) {


Nit: the new styleguide asks for elementToUse = undefined instead

gmajoulet · 2018-03-16T19:00:32Z

src/services.js

+   * @param {!Window} win
+   * @return {?Promise<?../extensions/amp-story/0.1/messages.MessageService>}
+   */
+  static messageServiceForOrNull(win) {


Since no other extension is planning on using this service, we have no risk of race conditions. Could we just get it from the synchronous messageService method?

Don't forget about me 🙂

+1. Also, this isn't stories-specific, so in theory, other people could use this for their strings too (/cc @cathyxz, does lightbox gallery have strings?)

Nope, don't have strings on lightbox gallery (yet), but we may end up with aria strings in all components for accessibility.

alanorozco

LGTM although the format is a little verbose.

I think tables / ids should be built from source, akin to:

msg('Hi, I'm a message.')

...in plain English so that we can automatically invalidate stale translations, etc. The build step would then create pointers to unique strings and all that.

I don't think this is urgent, but I thought it might be worth putting out there.

alanorozco · 2018-03-16T23:24:14Z

extensions/amp-story/0.1/amp-story-desktop.css

@@ -335,6 +334,7 @@ div.i-amphtml-story-top {
  margin: auto!important;
  color:#fff!important;
  box-sizing: initial !important;
+  text-transform: uppercase!important;


alanorozco · 2018-03-16T23:25:49Z

extensions/amp-story/0.1/messages.js

+   */
+  getLanguageCodesForElement_(element) {
+    const languageEl = closest(element, el => el.hasAttribute('lang'));
+    const languageCode = languageEl ? languageEl.getAttribute('lang') : null;


nit: you can probably fallback to empty string here.

getAttribute can return null if the attribute is not present, so I felt null was more consistent; languageCode is either the value of the lang attribute, or null if there wasn't one.

dreamofabear

Drive-by bikeshed: "Message" is confusing since it's also a verb. Recommend "I18n" or "LocalizedStrings".

newmuis · 2018-03-19T19:56:27Z

@choumx Done
@alanorozco I think you're right that's a better end goal, but we need a build step for that, and we don't currently have it. This, at least, gets all the messages in a centralized place, so other refactors will be easier (i.e. they won't need to rip strings out that ended up in CSS somehow). Also, if we allow multiple langs in the same doc, the build step becomes very complicated 😄

newmuis · 2018-03-21T17:28:19Z

Fixed type errors, renamed "messages" (and all related names) to "localized strings", and added the ability to create pseudo-locales, which will help us out for testing. We can expand pseudo-locale supoprt in a separate future PR, but this at least lets you verify that the string(s) you are using are indeed being passed through the LocalizationService, and are not hard-coded.

…add missing files

…ngs in the current UI.

…oMessage to getMessage

… strings in local dev

gmajoulet

LGTM

gmajoulet · 2018-03-23T20:47:35Z

extensions/amp-story/0.1/_locales/default.js

+  },
+  [LocalizedStringId.AMP_STORY_SHARING_CLIPBOARD_FAILURE_TEXT]: {
+    string: ':(',
+    description: 'String shown in a toast to inform the user that a link ' +


Nit: a translator would probably not know what a toast is

gmajoulet · 2018-03-23T20:50:17Z

extensions/amp-story/0.1/localization.js

+ *
+ * Next ID: 22
+ *
+ * @const @enum {string}


Just curious: any reason why we want strings instead of numbers?

😦

Yeah. We directly use these as object keys in the _locales/*.js files, and object keys have to be strings. If you let this be @enum {number} they get coerced to strings when used as keys, but then you get all kinds of nasty type errors when you try to use Object.keys because they're all strings, but anything that was accepting an ID is expecting a number.

newmuis requested review from alanorozco, gmajoulet and dreamofabear March 16, 2018 17:49

googlebot added the cla: yes label Mar 16, 2018

gmajoulet approved these changes Mar 16, 2018

View reviewed changes

alanorozco approved these changes Mar 16, 2018

View reviewed changes

alanorozco changed the title ~~First pass at internationalizing amp-story~~ ✨First pass at internationalizing amp-story Mar 16, 2018

dreamofabear reviewed Mar 17, 2018

View reviewed changes

newmuis force-pushed the amp-story-messages branch from bec55eb to 3075dd5 Compare March 21, 2018 17:24

newmuis changed the title ~~✨First pass at internationalizing amp-story~~ 🌐✨First pass at internationalizing amp-story Mar 22, 2018

newmuis added 15 commits March 23, 2018 16:49

First pass at messages

ecedc19

Update to messages

211dd93

Update SimpleTemplates to work with message IDs instead of text, and …

6ecf3b0

…add missing files

Add test for setting message with message service.

68617f1

Add English strings for messages that cannot be conveyed without stri…

ae96905

…ngs in the current UI.

Use textContent instead of innerText.

10234e0

Migrate some messages to message service; change from setTextContentT…

e8c30e2

…oMessage to getMessage

Migrate strings to use messages.

f18ab92

Review fixes

791be5f

Rename Message -> LocalizedString

dfdea28

Update types

30f2a69

Add pseudolocalization (generally), and an en-XA bundle for amp-story…

819de89

… strings in local dev

Move en-XA bundle out of local dev for testing on e.g. Heroku

1615cb3

Fix unlocalized strings

a19def0

More renames message => localized string

5c422da

newmuis force-pushed the amp-story-messages branch from 3075dd5 to 5c422da Compare March 23, 2018 20:50

gmajoulet approved these changes Mar 23, 2018

View reviewed changes

Update descriptions in en.js and drop descriptions from default.js

18037ea

newmuis merged commit fabd75e into ampproject:master Mar 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌐✨First pass at internationalizing amp-story #14051

🌐✨First pass at internationalizing amp-story #14051

newmuis commented Mar 16, 2018 •

edited

newmuis commented Mar 16, 2018

gmajoulet Mar 16, 2018

gmajoulet Mar 16, 2018

calebcordry Mar 16, 2018

newmuis Mar 16, 2018

cathyxz Mar 16, 2018

alanorozco left a comment •

edited

alanorozco Mar 16, 2018

alanorozco Mar 16, 2018

newmuis Mar 19, 2018

alanorozco Mar 19, 2018

dreamofabear left a comment

newmuis commented Mar 19, 2018

newmuis commented Mar 21, 2018

gmajoulet left a comment

gmajoulet Mar 23, 2018

newmuis Mar 23, 2018

gmajoulet Mar 23, 2018

newmuis Mar 23, 2018

🌐✨First pass at internationalizing amp-story #14051

🌐✨First pass at internationalizing amp-story #14051

Conversation

newmuis commented Mar 16, 2018 • edited

newmuis commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

newmuis commented Mar 19, 2018

newmuis commented Mar 21, 2018

gmajoulet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis commented Mar 16, 2018 •

edited

alanorozco left a comment •

edited