-
Notifications
You must be signed in to change notification settings - Fork 110
add i18n support #23
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
base: master
Are you sure you want to change the base?
add i18n support #23
Conversation
This is looking good. I made some simple comments and just curious what your thoughts are on them. |
@DuffMck , thoughts on my comments? |
Sorry, I haven't find your comments... can you give me the link to the file/s where they are? |
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.
Looks like I never started the review. My bad.
'THU': 'Thursday', | ||
'FRI': 'Friday', | ||
'SAT': 'Saturday' | ||
'en': { |
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.
I think rather than these being localized individually, these should just be the lookup keys in the provided translations map so that way there is only one place people have to go to add translations.
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.
I agree, next week I hope to have enough time change this one
@@ -705,7 +706,87 @@ var CronGenTimeSelect = function CronGenTimeSelect($scope, cronGenService) { | |||
}; | |||
CronGenTimeSelect.$inject = ["$scope", "cronGenService"]; | |||
|
|||
angular.module('angular-cron-gen', []).service('cronGenService', CronGenService).component('cronGenTimeSelect', { | |||
angular.module('angular-cron-gen', ['pascalprecht.translate']).config(["$translateProvider", function ($translateProvider) { | |||
$translateProvider.translations('en', { |
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.
I haven't used this library (we rolled our own i18n lib internally before something like this existed). Should we namespace these localization keys? Can this blow away someone else's localizations? Does this merge
into their existing english/Italian translations (hopefully it doesn't blow it away)?
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.
I think that it's possible, but I must check. Probably I should find some more smart way to passing translations I will make some tries!
'ngInject'; | ||
|
||
this.parsedOptions = this.mergeDefaultOptions(this.options); | ||
|
||
$translate.use(this.parsedOptions.language); |
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.
Since they are going to have to require angular translate in their project, would it make sense to just remove the language
setting from here and just use whatever has been previously set and if someone wants to configure it they just have to call that prior to component initialization? Does it choose a reasonable default if use
isn't called (again, not an expert in this library)?
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.
it has sense, the solution of the next point probably solve this one too.
Hi,
I've introduced support for internationalization and Italian translation, if you like it you can merge my code!
Inside the option object now you can find the field "language" where users can only choose it|en (default) right now. More languages can maybe be added by someone else.
By,
David