assemble / grunt-assemble-i18n

Assemble middleware for adding i18n support to projects.
24 stars 8 forks source link

Add translation keys to page context #33

Closed ain closed 10 years ago

ain commented 10 years ago

Add lodash variable support in YFM, e.g. <%= i18n.key %> Resolve #12 Feature: i18n lodash variable templates in front-matter

ain commented 10 years ago

@doowb @jonschlinkert @LaurentGoderre calling for a review :)

doowb commented 10 years ago

Is this working the way you expect? All of the i18n.title contents are in french. The way you're doing this, I would expect you to use something like... i18n.en.title for the specific language. Or am I missing something?

ain commented 10 years ago

Yup, you're right, the behaviour at this point is erroneous.

What I had in mind is to get all translation keys of current language into the page context. Could add an option for global keys, but since a page has language set, I think disregarding defining a language in key path would be reasonable.

ain commented 10 years ago

@doowb I added <%= i18n[language].title %> which should be more flexible and suffice. Makes it also possible to go after other languages in a page of a different language. Since language is set in the context anyway, it's not hard to get it going.

We could of course alias the currently active language under i18n directly, e.g. i18n.title, but on the second thought, it wouldn't be as robust creating unnecessary complexity. Ideas?

rauberdaniel commented 10 years ago

:+1:

ain commented 10 years ago

@doowb @jonschlinkert @LaurentGoderre calling for a final review :)

LaurentGoderre commented 10 years ago

Looks good to me.

doowb commented 10 years ago

@ain I'll take a look at this soon, but it might not be until this weekend.

mauron85 commented 10 years ago

Hi I tried this patch. And found another issue when looping over pages the variable i18n.title is not substituted for all pages, only for page that is defining that variable.

I have pricing page with title defined: <%= i18n[language].Pricing %> When I generate navigation with loop over all pages. Only for pricing page I get correct data.title, for all other pages I'm getting <%= i18n[language].Pricing %>

                {{#each pages}}
                    {{#is data.language ../language}}
                    <li{{#if this.isCurrentPage}} class="active"{{/if}}>
                        <a{{#unless this.isCurrentPage}} class="page-scroll"{{/unless}} href="{{relative dest this.dest}}">{{data.title}}</a>
                    </li>
                    {{/is}}
                {{/each}}
mauron85 commented 10 years ago

The interesting fact about this thing is that last page (of each loop over all pages) has all translation, previous is missing one, ... I think it's because i18n process function is called one by one for each page template. The solution will be that preload all page translation in advance and then call process function.

LaurentGoderre commented 10 years ago

Well what do you know. This issue is hitting us too right now. One thing that bothers me is that this would only work if using this middleware. There would be scenario where developer would not need this middleware

Would it be possible to achieve this in the helper instead?

LaurentGoderre commented 10 years ago

@doowb what do you feel about adding i18n to the core context?

jonschlinkert commented 10 years ago

would one of you gentlemen be so kind as to add these use cases to https://github.com/assemble/assemble-i18n-examples, so we can collaborate on solving these issues? (@LaurentGoderre, didn't we have a test project for this stuff already?)

jonschlinkert commented 10 years ago

@LaurentGoderre what do you mean by "core context". do you mean built-in, as something we give first class treatment to?

LaurentGoderre commented 10 years ago

Here's what I was thinking either integrate i18n as a core feature of assemble or if possible, move the i18n helper to the i18n middleware so that it's all packaged together

doowb commented 10 years ago

Yes, you can add a helper through the middleware, and I think that's where it should be done. I don't think it should be a core feature of assemble, but it might be something to consider as a default plugin (I guess that can mean core too).

This repo was supposed to be examples on how to get i18n working with assemble. There are different possible ways of adding i18n data in the Gruntfile. It just happened to turn into an actual middleware that can be used too. I think the middleware itself needs to be documented more and I also think this is a good one to move into v0.6 sooner than some of the others.

I'll take a look later today and figure out the migration path.

LaurentGoderre commented 10 years ago

I think since we put all this effort in, there is nothing wrong with our joint approach to be the default i18n way yet it doesn't stop anyone from creating another middleware.

Can you give me a hint on how to add the helper via the middleware? Once we do this, we can solve everyone's issue and add a full test suite for i18n.

/cc @ain are you in?

doowb commented 10 years ago

@LaurentGoderre in this version (v0.4) of assemble you can add helpers via assemble.engine.registerFunctions().

I would probably place the code inside this if statement:

params.assemble.engine.registerFunctions({
  foo: function (str) {
    return 'foo ' + str;
  },
  bar: function (str) {
    return 'bar ' + bar;
  }
});

I think since we put all this effort in, there is nothing wrong with our joint approach to be the default i18n way yet it doesn't stop anyone from creating another middleware.

I agree. I don't have as much localization experience, so this is very useful for me and I'm sure it'll be a default.

LaurentGoderre commented 10 years ago

Btw I have to congratulate you guys for this project and community. It's really nice to be able to make some positive changes!

LaurentGoderre commented 10 years ago

Btw, this is not published on npm. Should it be?

doowb commented 10 years ago

Thanks!

Btw, this is not published on npm. Should it be?

It's on npm as assemble-contrib-i18n. I'm not sure when the repo was renamed, but it was probably in anticipation for v0.5. The package.json still has assemble-contrib-i18n so doing npm publish will still be correct.

LaurentGoderre commented 10 years ago

Aaah good to know! Thanks!

LaurentGoderre commented 10 years ago

I'm keeping it out of the loop because I want that just by adding the plugin you would get the helper. This would be helpful for cases where you don't need the full logic of the middleware

LaurentGoderre commented 10 years ago

Still I'm not getting it to work yet for some reason

LaurentGoderre commented 10 years ago

@doowb I'm getting an error. It never loads the plugin because the helper is missing. It seems I can't add the helper early enough

doowb commented 10 years ago

Do you have a repo I can try?

LaurentGoderre commented 10 years ago

Not really. I just deleted the helpers-i18n.js from my node_modules and add the lines you suggested but it never reaches it.

LaurentGoderre commented 10 years ago

I created a branch here with my changes:

https://github.com/assemble/assemble-middleware-i18n/tree/add-i18n-helper

LaurentGoderre commented 10 years ago

This is messed up...it work in this repo but not the other one....

LaurentGoderre commented 10 years ago

@doowb nevermind....stupid error. It works great!!!

doowb commented 10 years ago

Cool.

LaurentGoderre commented 10 years ago

Merged in manually