-
Notifications
You must be signed in to change notification settings - Fork 12k
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
expose ngc i18n options #3098
expose ngc i18n options #3098
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
@vicb could you confirm (we can talk about it tomorrow) that this change will work with the next iterations of i18n? |
</file> | ||
</xliff>`)) | ||
.then(() => appendToFile('src/app/app.component.html', | ||
'<h1 i18n="User welcome|An introduction header for this sample">Hello i18n!</h1>')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove User welcome|
(because meaning should seldom be used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did not know that.
Also couldn't find anything about using it seldom on the angular cookbook.
yep let's talk tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Just one comment to add another test and it should be good!
'<h1 i18n="An introduction header for this sample">Hello i18n!</h1>')) | ||
.then(() => ng('build', '--aot', '--i18n-file', 'src/locale/messages.fr.xlf', '--i18n-format', | ||
'xlf', '--locale', 'fr')) | ||
.then(() => expectFileToMatch('dist/main.bundle.js', /Bonjour i18n!/)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a negative test? Running ng build
on the same project without --locale
should not contain Bonjour i18n
.
@hansl is this ok now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks great. LGTM
I'll rebase and merge this soon. |
Just a little nudge. Thanks! |
@hansl Is there anything we can do to get this PR into the next release? |
Sorry, not beta 22, but beta 23. I'll rebase this today. |
Not sure if there's something wrong with the PR, but I've cloned the repo, linked it with "npm link", but when I run it, it is looking for a folder in someone's user directory @tdesmet @hansl ?
I have no clue why this is happening? I've also rebased the solution the latest Angular-CLI version which has updated dependencies to Angular Compiler, but same error...:
|
Have you tried putting the path between double quotes?
|
Nope, same problem :( @tdesmet
|
@rolandoldengarm that's probably a typescript problem, yesterday a few of those were cropping up. Make sure you are using |
@tdesmet some recent changes introduced a lot of conflicts with this PR, can you rebase and fix them? |
Done |
@filipesilva I've tried on a clean machine, with TS 2.0.10, and still the same error. |
sorry for bothering you again, but when will this PR be merged and released? We are blocked by this; I don't know how to implement i18n + AOT without this PR being merged. |
@hansl @filipesilva can you guys merge this please? |
supressSizes: boolean; | ||
baseHref?: string; | ||
aot?: boolean; | ||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git merge issue?
There are many more below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, I'm sorry these .orig files should never have been committed.
These are artifacts from merging and shouldn't have been added I'm used to them being ignored.
Anyway I Removed them and everything should be good.
Hi guys, thanks for the good work! Do you know when it will be merged and released? Are there any chances for the next week? Thanks! |
Alright. Merging this. I am about to change the plugin for the next release and this LGTM, so expect it to come in the next release (which will also be ng 2.3 compatible). |
@tdesmet could we also have an angular-cli global option for these? would that be useful? |
@hansl what do you mean? |
Would it make sense to have Basically are those flags likely to change between two calls of |
Format stays the same but locale and file do not in this implementation. File is translated messages for the locale. So if locale is fr then file is messages.fr.xlf anf if locale is de then file is messages.de.xlf. I'm thinking we could probably make this easier by determining the file name by the given locale. Bu this would be less flexible as all users need to follow the same naming pattern. Currently in my app I have a locales directory and each locale is a subdirectory containing a messages.locale.xlf file. My build process scans the locales directory and executes the build command for each found subdirectory. For me it would be great if we could implement something similar in the cli. But not sure if this workflow would work for others... |
Having a CLI specific workflow is okay with me, as long as it’s easy and
transparent to the user. Let’s discuss more about this in a separate issue;
it’s probably not something coming to CLI final but I’m definitely
interested in making it easy for users to translate their apps in the
future! For now, having a broken workflow is probably good enough.
Thanks again for the PR!
…--
Hans Larsen
On December 14, 2016 at 1:25:20 PM, Tom De Smet ([email protected]) wrote:
Format stays the same but locale and file do not in this implementation.
File is translated messages for the locale. So if locale is fr then file is
messages.fr.xlf anf if locale is de then file is messages.de.xlf.
I'm thinking we could probably make this easier by determining the file
name by the given locale. Bu this would be less flexible as all users need
to follow the same naming pattern.
Currently in my app I have a locales directory and each locale is a
subdirectory containing a messages.locale.xlf file. My build process scans
the locales directory and executes the build command for each found
subdirectory.
By typing this I realise I could just name all xlf files the same since
they all live in seperate directories.
For me it would be great if we could implement something similar in the
cli. But not sure if this workflow would work for others...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3098 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AApn8RIRM0vPJITmGS4fzUNhjTcRUZvrks5rIF6_gaJpZM4Kun4->
.
|
Also available on the webpack plugin.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This allows you to pass the i18n file, format and locale to ngc from the cli command line