Lang select final #1109

richiethomas · 2017-06-22T21:41:00Z

Addresses #1094

Changes proposed in this pull request:

Add a dropdown-select element to the settings modal, viewable by clicking the gear icon in the navbar
The dropdown includes a hard-coded list of languages (for now just 2- English and French), and upon making a selection the language of any text under Handlebars i18n control will immediately update to the new language

NOTE- there are still many elements in the Handlebars views that are either:

a) hard-coded as English, or
b) controlled by i18n but require a page refresh because the i18n logic is in the component, not the view template

In the first case, these will need to be refactored to be controlled by the i18n package. That's pretty straightforward.

In the 2nd case, this is a little more difficult. The reasons for putting the i18n logic vary from component to component, so these refactorings will need to be addressed on a case-by-case basis. For an example, see the logic in app/inventory/route.js, which includes a translation for the inventory received button text. There is a function additionalButtons which contains information on the button action, button text, classes, etc. which seems to be should be extracted into the view, but this case and others like it require a bit of surgery and I thought it best to get this PR up first.

NB: I was able to extract the button text in the above example from hard-coded english to a translation, but when I try to make it a computed property, the button text renders as empty in the browser. Not sure why that is, but hopefully the issue will be irrelevant if and when I can extract the i18n logic into the view itself using the "t" view helper.

I'd appreciate any feedback on additional test coverage desired, or on refactoring the code itself to make it more idiomatic for Ember, but any feedback will be considered. :-)

Also, I'll add screenshots of the functionality as it stands now to the Github issue and the comments below, but it may also be advisable to pull down the code from the branch and test it manually on your local machine, before giving a final go-ahead.

cc @HospitalRun/core-maintainers

…in navbar

…c i18n text; changed dynamic pageTitles for patient reports and patient index routes to computed properties so that they update instantly instead of after page refresh

richiethomas · 2017-06-22T21:58:37Z

Screenshots below:

Before clicking the "Settings" Gear:

Clicking the "Settings" gear shows the language select dropdown:

Selecting a language:

Language changed from English to French (note the change in sidebar and table header translations, but page title has not changed):

After page refresh, page title has changed too:

richiethomas · 2017-06-22T22:02:57Z

P.S.- ignore the <script>alert("Hi")</script> in the Patient Name column. I was doing some security testing. :-p

Also, an update on the inventory received button text- after looking at the file app/templates/section.hbs, it should be trivial to move the translation logic to the view and just pass the translation key from the route instead of the pre-translated text.

…on i18n service Changed 'Lookup Lists' page title to computed property which depends on i18n service Changed 'Shortcodes' page title to computed property which depends on i18n service

…depends on i18n service Changed 'Medication Requests' page title to computed property which depends on i18n service Changed 'Appointments Calendar' page title to computed property which depends on i18n service (NOTE- French translation is currently missing this key/value pair) Changed 'Appointments This Week' page title to computed property which depends on i18n service Changed 'New Appointment' buttontext in 'Scheduling' section to computed property which depends on i18n service

… on i18n service Changed 'Appointments Search' page title to computed property which depends on i18n service Changed 'Theater Schedule' page title and button text to computed properties which depends on i18n service Changed 'Appointments Today' page title to computed property which depends on i18n service

jkleinsc

@richiethomas generally speaking this looks good to me. There are a few tweaks that I've pointed out below. Also, don't worry about changing all of the components/pages in this PR. They can be handled as separate issues/PRs.

jkleinsc · 2017-06-26T14:51:26Z

app/admin/lookup/route.js

 export default AbstractIndexRoute.extend({
 hideNewButton: true,
- pageTitle: t('admin.lookup.pageTitle'),
+ pageTitle: computed('i18n', () => {


This should be computed('i18n.locale'....

jkleinsc · 2017-06-26T14:51:41Z

app/admin/textreplace/route.js


 export default AbstractIndexRoute.extend({
- pageTitle: t('admin.textReplacements.pageTitle'),
+ pageTitle: computed('i18n', () => {


This should be computed('i18n.locale'....

jkleinsc · 2017-06-26T14:52:02Z

app/appointments/search/route.js

 export default AppointmentIndexRoute.extend(DateFormat, {
 editReturn: 'appointments.search',
 filterParams: ['appointmentType', 'provider', 'status'],
 modelName: 'appointment',
- pageTitle: t('appointments.searchTitle'),
+ pageTitle: computed('i18n', () => {


This should be computed('i18n.locale'....

jkleinsc · 2017-06-26T14:52:13Z

app/appointments/theater/route.js


 export default AppointmenCalendarRoute.extend({
 editReturn: 'appointments.theater',
- newButtonText: t('appointments.buttons.scheduleSurgery'),
- pageTitle: t('appointments.titles.theaterSchedule'),
+ newButtonText: computed('i18n', () => {


These should be computed('i18n.locale'....

jkleinsc · 2017-06-26T14:52:21Z

app/appointments/today/route.js

 export default AppointmentIndexRoute.extend({
 editReturn: 'appointments.today',
 modelName: 'appointment',
- pageTitle: t('appointments.todayTitle'),
+ pageTitle: computed('i18n', () => {


This should be computed('i18n.locale'....

jkleinsc · 2017-06-26T14:53:02Z

app/components/language-select.js

+ return Object.keys(this.get('languageMap'));
+ }),
+ onFinish: null,
+ languageMap: Ember.computed(() => {


This should be localized text. Also, why is this duplicated in app/controllers/navigation.js?

Fixed. Duplicated code removed and language names localized. One callout- I was able to remove the value attribute from the select-list without any apparent adverse effects. Not sure why this was the case, since I thought value maps to the value attribute of the option tag. Does this get overridden if the optionValuePath and optionLabelPath attrs are used instead?

Also, I added localized English/Spanish/French values for the labels "English", "Spanish", and "French", but stopped short of adding the respective keys for all supported languages (i.e. the Russian translation for the label "Urdu", etc.).

@richiethomas value is used to set the currently selected value of the drop down. It is a way of setting which option is selected.

jkleinsc · 2017-06-26T14:59:12Z

app/components/language-select.js

+ }),
+ selectedLanguage: null,
+
+ _setUserLanguageChoice(language) {


The record that is being written to the configDB isn't used anywhere. Are you planning on using it for later code?

Hmmm, I thought the record being written was in fact being used, specifically by the afterModel hook in app/routes/application.js. My intent here in _setUserLanguageChoice was to write the user's language preference to the database, and then if the user logs out and later logs back in, their language choice is retrieved from the DB in the above-mentioned afterModel hook. I tested it in the browser and the app's behavior is what I expected, but am I confused about how the behavior works under the hood?

jkleinsc · 2017-06-26T15:00:25Z

app/imaging/index/route.js

 export default AbstractIndexRoute.extend({
 modelName: 'imaging',
- pageTitle: t('imaging.pageTitle'),
+ pageTitle: computed('i18n', () => {


This should be computed('i18n.locale'....

jkleinsc · 2017-06-26T15:01:53Z

app/inventory/route.js

 import AbstractModuleRoute from 'hospitalrun/routes/abstract-module-route';
 import FulfillRequest from 'hospitalrun/mixins/fulfill-request';
 import InventoryId from 'hospitalrun/mixins/inventory-id';
 import InventoryLocations from 'hospitalrun/mixins/inventory-locations'; // inventory-locations mixin is needed for fulfill-request mixin!
 export default AbstractModuleRoute.extend(FulfillRequest, InventoryId, InventoryLocations, {
 addCapability: 'add_inventory_item',
+ buttonText: t('navigation.subnav.inventoryReceived'),


This buttonText should use a separate localization entry, not reuse the one from the subnav (the text is intentionally lowercase for the button).

Removed this as it was mostly a nice-to-have and I don't want to block the story.

jkleinsc · 2017-06-26T15:02:48Z

app/routes/application.js

@@ -106,6 +106,10 @@ let ApplicationRoute = Route.extend(ApplicationRouteMixin, ModalHelper, SetupUse
 afterModel() {
 set(this.controllerFor('navigation'), 'allowSearch', false);
 $('#apploading').remove();
+ this.get('config.configDB').get('current_user').then((user) => {
+ let language = user.i18n || 'en';


This value isn't set anywhere.

Isn't it getting set in the addition I made to the _setUserLanguageChoice(language) method of app/components/language-select.js?

_setUserLanguageChoice(language) saves a new record in the configDB, with the id of the record being the user, but here you are just fetching the current_user, which has an id of 'current_user', not the id of the user. Does that make sense?

You mentioned _setUserLanguageChoice(language) saves a new record in the configDB. I had a different understanding of how it works. I believe _setUserLanguageChoice(language) first fetches the current_user from the configDB, i.e. configDB.get('current_user'). That returns a promise, and when that's resolved successfully it passes a user object to the success callback. That callback takes the user object and uses its _id attribute to send a put call to the configDB. That put call looks like this:

configDB.put({ i18n: language, _id: user._id, _rev: user._rev, value: user.value });

Because I specified the _id attribute of the user, my understanding is that PouchDB will overwrite the existing record's attributes (i.e. an update operation) instead of creating a new record. My intent was to follow the guide here:

https://pouchdb.com/guides/documents.html#updating-documents–correctly

As a tangent to this conversation, to make my intent clearer I refactored the code a bit in _setUserLanguageChoice(langauge) (now called _setUserLanguage(language) for succinctness).

I'm still learning how both HospitalRun and PouchDB work, so I could be wrong. Happy to hop on a Slack chat or something if it's easier to talk that way.

When you call configDb.get('current_user') it pulls a record with an _id === 'current_user', but when you save the record with user._id the id of that record is going to be something like org.couchdb.user:couchadmin, not current_user. Does that make sense?

…ller

…ndly; labels are hard-coded but map to entries in locales/translations.js files

… translation keys for language names still missing

…s back in the future as a separate story

jkleinsc · 2017-07-03T14:43:23Z

app/components/language-select.js

- _rev: user._rev,
- value: user.value
- });
+ user.i18n = language;


This is the change I was expecting. This will work!

jkleinsc · 2017-07-03T15:10:47Z

@richiethomas I'm going to go ahead and merge this in. Thanks for your work on this!

richie-thomas added 3 commits June 22, 2017 12:28

Add persistent, user-specific i18n toggle to settings dropdown modal …

ddf2904

…in navbar

Changed hard-coded English 'inventory received' button text to dynami…

ce55269

…c i18n text; changed dynamic pageTitles for patient reports and patient index routes to computed properties so that they update instantly instead of after page refresh

Renamed 'lang' attr on current user to 'i18n' to be more semantic

1a30f93

richiethomas mentioned this pull request Jun 22, 2017

A button for switch between language... #1094

Closed

richie-thomas added 3 commits June 23, 2017 13:51

Changed 'Custom Forms' page title to computed property which depends …

59dcaf8

…on i18n service Changed 'Lookup Lists' page title to computed property which depends on i18n service Changed 'Shortcodes' page title to computed property which depends on i18n service

jkleinsc suggested changes Jun 26, 2017

View reviewed changes

richie-thomas added 8 commits June 27, 2017 08:20

Fixed i18n dependency inclusion issues for 5 routes

34fb099

Add i18n language name translations

39d1e19

Remove superfluous 'languages' computed property in navigation contro…

abd1b19

…ller

Fix i18n dependency on imaging index route

0ce67ca

Refactor language labels in language-select component to be i18n-frie…

b60f7c4

…ndly; labels are hard-coded but map to entries in locales/translations.js files

Languages in language-select dropdown component are now dynamic; some…

2a75dbb

… translation keys for language names still missing

Add empty translation keys for language names included in i18n locales

5e750e2

Removed i18n translation of 'inventory received' button; will add thi…

dda0187

…s back in the future as a separate story

richiethomas force-pushed the lang-select-final branch from 7212ac2 to a555be8 Compare June 27, 2017 18:04

richie-thomas added 3 commits June 27, 2017 14:08

Fix test failures

a555be8

Refactored 'setUserLanguageChoice' name to 'setUserLanguage'

4d64502

Refactored '_setUserLanguage' to be less verbose

b76a231

jkleinsc reviewed Jul 3, 2017

View reviewed changes

Merge branch 'master' into lang-select-final

fcec0c3

jkleinsc approved these changes Jul 3, 2017

View reviewed changes

jkleinsc merged commit 767e773 into HospitalRun:master Jul 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lang select final #1109

Lang select final #1109

richiethomas commented Jun 22, 2017 •

edited

Loading

richiethomas commented Jun 22, 2017 •

edited

Loading

richiethomas commented Jun 22, 2017 •

edited

Loading

jkleinsc left a comment

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

richiethomas Jun 27, 2017

jkleinsc Jul 3, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017

jkleinsc Jun 26, 2017

richiethomas Jun 27, 2017 •

edited

Loading

jkleinsc Jun 27, 2017

richiethomas Jun 28, 2017 •

edited

Loading

jkleinsc Jul 3, 2017

jkleinsc Jul 3, 2017

jkleinsc commented Jul 3, 2017

Lang select final #1109

Lang select final #1109

Conversation

richiethomas commented Jun 22, 2017 • edited Loading

richiethomas commented Jun 22, 2017 • edited Loading

richiethomas commented Jun 22, 2017 • edited Loading

jkleinsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richiethomas Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richiethomas Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkleinsc commented Jul 3, 2017

richiethomas commented Jun 22, 2017 •

edited

Loading

richiethomas commented Jun 22, 2017 •

edited

Loading

richiethomas commented Jun 22, 2017 •

edited

Loading

richiethomas Jun 27, 2017 •

edited

Loading

richiethomas Jun 28, 2017 •

edited

Loading