formatjs / handlebars-intl

Handlebars helpers for internationalization.
http://formatjs.io/handlebars/
Other
265 stars 28 forks source link

Simplified #20

Closed ericf closed 10 years ago

ericf commented 10 years ago

This simplifies how the helpers are used to create a composition system for app code to use.

An {{#intl}} block is used to setup a the data.intl which will propagate through all helpers and partials. Once setup, the {{intlNumber}}, {{intlDate}}, and {{intlMessage}} helpers can be used and they don't need to be concerned with setting up locales.

{{#intl locales=intl.locale messages=intl.messages}}
    <p>
        {{intlMessage intlName="USERS_BOOKS"
            firstName=user.firstName lastName=user.lastName
            numBooks=user.numBooks dateBooks=user.dateBooks}}
    </p>

    <p>{{intlMessage "The number is: {num, number, integer}" num=2000}}</p>

    <p>{{inltNumber 1000 style="currency" currency="USD"}}</p>
{{/intl}}

Todos:

drewfish commented 10 years ago

In {{intlMessage intlName="USERS_BOOKS"}} the intlName parameter name is confusing to me. Is this intended to specify the message to use? If so what about message=... instead?

drewfish commented 10 years ago

additional TODO:

drewfish commented 10 years ago

Is {{#intl locales= intended to hold one locale or a list?

If it's a list how does that relate to the messages= parameter, which I was assuming is the messages for just the single specified locale.

caridy commented 10 years ago

@drewfish about the intlName, ideally that will be removed pretty soon, and the users will be doing sub-expressions like this:

 {{intlMessage (intlGet "USERS_BOOKS")
            firstName=user.firstName lastName=user.lastName
            numBooks=user.numBooks dateBooks=user.dateBooks}}

but there is a bug in handlebars that prevents us from doing that. YUI will have to be updated with the next release of handlebars, then we will be able to deprecate intlName hash name.

drewfish commented 10 years ago

So what does intlGet do? If it gets a registered message I think it should be intlMessage (or maybe intlMsg).

caridy commented 10 years ago

it resolves a message from the data.intl.message collection.

drewfish commented 10 years ago

Yeah, then intlMessage (or intlMsg) makes more sense to me. As a user intlGet means nothing and I'd have to read in the documentation to know what the heck it is and what it does.

drewfish commented 10 years ago

BTW I don't forsee that the dust helper will need this complexity, since it doesn't have (or need) this extra data side-band data store.

drewfish commented 10 years ago

Will this new approach also let the user use the intl block to set other default parameters, for example currency in the following...

{{#intl locales=intl.locale messages=intl.messages currency="USD" }}
    <p>{{inltNumber 1000 style="currency" }}</p>
{{/intl}}
ericf commented 10 years ago

In {{intlMessage intlName="USERS_BOOKS"}} the intlName parameter name is confusing to me. Is this intended to specify the message to use? If so what about message=... instead?

@drewfish this is a stop-gap. There's currently a bug in Handlebars that I'm working on fixing, but the idea was to use one of the following syntaxes where the message is passed as an argument from either the context, data, or looked up by path name:

{{intlMessage i18n.USERS.BOOKS}}
{{inltMessage @intl.messages.USERS.BOOKS}}
{{intlMessage (intlGet "USERS.BOOKS")}}

The last version is the best because it works in partials, but a bug in Handlebars prevents this to be used in some circumstances (working to fix that now).

drewfish commented 10 years ago

My main argument is that the string intlGet is confusing to me as an end-user. It's name should be more descriptive.

apipkin commented 10 years ago

Do we want to go ahead and merge this in and move forward with this approach? Merging this in would make future PRs more legible.

ericf commented 10 years ago

@apipkin I still need to update the tests and docs.

apipkin commented 10 years ago

+1 for merging this in before tests and docs are updated

apipkin commented 10 years ago

I was stepping through some of the tests. It appears doing {{intlNumber 4000 locale="de-DE"}} will not use the "de-DE" locale. Is this intentional?

caridy commented 10 years ago

Yes, that's intentionally. Only the intl block can define internalization meta-data to avoid confusions. that means only intl can define locales, formatters, messages, etc.

apipkin commented 10 years ago

Okay!

ericf commented 10 years ago

@apipkin and the reason for that is that it uses options.data to propagate those intl-related settings down to all the other helpers and partials, without intermixing them with the templates rendering context.

apipkin commented 10 years ago

But, would that not still be beneficial to change the locale is just one item? I guess adding a new intl block wouldn't really be that big of a deal though

ericf commented 10 years ago

I've updated the unit tests and they now pass. @apipkin @caridy shall I merge this and we'll do the README update after all the changes are in?

apipkin commented 10 years ago

@ericf That sounds like an ideal situation to me. I can do merging if you like since you are traveling