Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Address #277: Localization for Imaging Module #283

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

alexpelan
Copy link
Contributor

-Added an initializer to inject the I18N service into all controllers, routes, and mixins
-Did not write any i18n-specific tests - don't have a lot of experience in how to automated test i18n, in this case knowing that the current tests didn't break when run with the 'en' locale is probably good enough? Would gladly write some if you have any suggestions.
-The Ember localization syntax can get kind of awkward sometimes, it doesn't feel very DRY. Take for example the subActions changes in app/imaging/route.js - you can't access the translation helper in the array because this isn't bound to the route , so you have to change it to a function. Not sure if there's a way around this, that method was recommended here: http://stackoverflow.com/a/33380796/265714

A couple of things I considered out of scope:
-I didn't localize the nav
-I made no effort to audit display of numbers / dates / etc in the proper locale - seems like we're just focused on translating strings for now.

@jkleinsc
Copy link
Member

@alexpelan Good work here.
As far as i18n specific tests, I think for now we are ok. Eventually I would like to add tests to verify that translations aren't missing, but I think that is outside the scope of this issue.
Using a computed property (i.e. function(){}.property()) is the accepted way to tackle the subActions.
You are correct that the other stuff you listed is out of scope for this issue.

If you can clean up the tests that are failing (looks like some simple formatting issues) and remove the //debugger from app/components/nav-menu.js, I think we can merge this PR.

@alexpelan
Copy link
Contributor Author

@jkleinsc Just force pushed and it looks like everything passed. I think you read the diff wrong - i removed the //debugger, didn't add it - unless that was a load-bearing //debugger :-)

@tangollama
Copy link
Member

+1 XP to @alexpelan for witty PR comments.

@jkleinsc
Copy link
Member

@alexpelan PR looks good. Yup... I misread that and yeah it wasn't a load-bearing //debugger!

jkleinsc added a commit that referenced this pull request Jan 26, 2016
Address #277: Localization for Imaging Module
@jkleinsc jkleinsc merged commit eeb00bd into HospitalRun:master Jan 26, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants