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

CHECKOUT-1234: Expose translation object #103

Merged
merged 2 commits into from
Feb 23, 2017

Conversation

davidchin
Copy link
Contributor

@davidchin davidchin commented Jan 6, 2017

What?

  • Expose translated strings as a JSON object, so that it can be directly consumed by UCO (and other JS apps). i.e.:
<script>
    window.language = {{{langJson 'checkout'}}};
</script>
{
    "locale": "en-US",
    "locales": {
        "checkout.address.city_label": "en",
        "checkout.address.city_required_error": "en",
        "checkout.customer.checkout_as_guest_text": "en",
        "checkout.customer.continue_as_guest_action": "en-US"
    },
    "translations": {
        "checkout.address.city_label": "City",
        "checkout.address.city_required_error": "City is required",
        "checkout.customer.checkout_as_guest_text": "Checking out as a <strong>Guest</strong>? You'll be able to save your details to create an account with us later.",
        "checkout.customer.continue_as_guest_action": "Continue as guest"
    }
}
  • Rename Localizer as Translator because the module only translates strings. AFAIK, localizer is an internal module so the change in interface should be ok.
  • Improve the test coverage of the translation module. Previously, you get this warning in the coverage report lib/localizer.js missing coverage on line(s): 21, 57, 120, 136.
  • Fix the way formatters are cached. Previously, we pass in a translations object as a parameter by reference and directly assign formatters to that object. Generally speaking, we shouldn't modify parameters that are passed in by reference.
  • Improve the performance of the function used to flatten the translation object. Previously, it does an extra loop which increases its complexity.
  • Move some private functions, such as data transformation and filtering, into task-specific modules.
  • Remove unneccessary dependency on hoek.

Why?

  • Currently UCO has its own translation system - gettext. We want to change that so it uses Stencil translation system. In order to do that, we need to expose the translated strings as a JSON object. So that it can be used to configure UCO.
  • Other changes are code quality improvements.

Testing / Proof

  • Unit

@bigcommerce/checkout @mcampa @lord2800 @mjschock

@mcampa
Copy link
Contributor

mcampa commented Jan 6, 2017

Nice refactor 👍

@davidchin davidchin force-pushed the translations branch 2 times, most recently from 4f0a7b4 to d8e62c1 Compare January 12, 2017 23:33
@davidchin
Copy link
Contributor Author

davidchin commented Jan 13, 2017

During testing, I discover that we're not formatting strings according to their locale when we merge multiple language files. It is an existing issue affecting the master branch at the moment.

Currently, language files get cascaded - region-specific, regular and fallback (English) language files get merged in that order. In order to format these merged translations correctly, we need to have a MessageFormat instance for each applicable locale. But we don't have that at the moment.

An example of this problem:

en.json

{ "items": "{count, plural, one{1 Item} other{# Items}}" }

zh.json

{ "days": "{count, plural, other{# 天}}" }

So if your preferred language is zh (Chinese), you'll get the following result.

{{lang 'days' count=1}} <!-- Prints "1 天" -->
{{lang 'items' count=1}} <!-- Prints "1 Items" -->

This is because Paper is using Chinese MessageFormat instance to format the fallback string, and Chinese doesn't have plurals. It only supports "other" http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html

Anyway, I've made a new commit addressing the issue above. @mcampa @lord2800 @icatalina @jordanwink201 @PascalZajac, can you please take a look again?

Copy link

@icatalina icatalina left a comment

Choose a reason for hiding this comment

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

👍

const Filter = require('./filter');
const LocaleParser = require('./locale-parser');
const Logger = require('../logger');
const Transformer = require('./transformer');

Choose a reason for hiding this comment

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

would be nice if these const were 🔤
🍹

@jordanwink201
Copy link

good stuff 👍

@Ubersmake
Copy link

💚

@davidchin davidchin merged commit 5c3721a into bigcommerce:master Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants