Skip to content
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

Add multilabel translations #47

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

sebastianbarry
Copy link
Contributor

Added mutlilabel translations to:

  • en.json
  • lo.json

I took the translations from: https://github.com/e-mission/nrel-openpath-deploy-configs/blob/main/label_options/usaid-laos-ev-label-options.json and https://github.com/e-mission/nrel-openpath-deploy-configs/blob/main/label_options/example-program-label-options.json


Another thing I noticed while making this change, are the https://github.com/e-mission/e-mission-translate/blob/master/es/i18n/trip_confirm_options-es.json files here in e-mission-translate. The issue I see, is that if we keep these files around, then the textual translations will effectively be duplicated, and if we update the multilabels translations in en.json, then we may forget to update the translations in trip_confirm_options-es.json - additionally, I'm not certain we are using this file anyway.

@sebastianbarry
Copy link
Contributor Author

This PR is associated with the issue: e-mission/e-mission-docs#1007
e-mission-phone PR: e-mission/e-mission-phone#1065

Copy link
Member

@Abby-Wheelis Abby-Wheelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this pop up and glanced over it, and I noticed two things in the Spanish translations.

es/i18n/es.json Outdated Show resolved Hide resolved
es/i18n/es.json Outdated Show resolved Hide resolved
@sebastianbarry
Copy link
Contributor Author

Thanks for the help @Abby-Wheelis !

lo/i18n/lo.json Outdated
Comment on lines 151 to 152
"e-auto_rickshaw":"ລົດ 3 ລໍ້ໄຟຟ້າ ຫລື ຕຸກຕຸກໄຟຟ້າ",
"auto_rickshaw":"ເດີນທາດ້ວຍ ລົດຕຸກຕຸກ ຫລື ລົດສາມລໍ້",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why these are here. The goal behind this feature, IIRC, is to put default translations (the ones that correspond to the default options) into the hard-coded, "included in app" translations. These modes are not standard, and are not part of the default mode options. So they should not be in the hardcoded translations, only in the overridden label config for the Lao project.

Copy link
Contributor Author

@sebastianbarry sebastianbarry Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make your proposal (to only have default "included in app" translations in the i18next files) the standard if you believe it will be better, but the advantage with having this double-layered structure for the translations is so that we can cover all options.

Continuing on the example you pointed out with e-auto_rickshaw and auto_rickshaw we have 2 ways to approach this:

1. Remove these non-standard translations from lo.json 2. Add these non-standard translations to en.json and es.json
Argument for By removing the translations from lo.json, we choose standardizing on the i18next files to only have the standard "included in app" translations. This means that we will not have "default" translations for programs whose special modes/purposes do not contain translations in other languages (i.e. Spanish will not have translations for e-auto_rickshaw and auto_rickshaw, so any devices set to Spanish that are part of the Lao program will not have translations for those modes) By adding the special translations to all i18next files, this solution provides us with the ability for Spanish devices to have e-auto_rickshaw and auto_rickshaw as part of the Lao program, but the drawback to this method is that it is less structured (the i18next files will still have all default included in app translations, but they may also have non-standard translations as well and there is not a rule for which non-default translations are included). In this implementation, the i18next multilabel translations are not "only default/included in app translations", in that they can contain translations for modes that may not be a part of the default list. This would leave the door open for new modes/purposes being added to any given program, to need a change to the phone code, since en.json is by default in the phone code ( + a change to e-mission-translate for es.json and lo.json), HOWEVER cases like this could be handled on a case-by-case basis where, when a new mode/purpose is introduced, we make the conscious decision to either 1. Add the new translation to the the i18next files, or 2. add the new translations to the dynamic label-options file. The advantage to this is that it gives us the option, and does not punish us for making the "wrong" decision.

Copy link
Collaborator

@shankari shankari Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spanish will not have translations for e-auto_rickshaw and auto_rickshaw, so any devices set to Spanish that are part of the Lao program will not have translations for those modes

Unless we include spanish translations in the custom labels file as well, which I would argue we should do if we are worried about users whose phones are set to Spanish in Laos.

To me, the basic mapping seems to be:

  • standard label: standard translation location
  • custom label: translation in the custom labels file

I don't see how this option punishes us for making the wrong decision.

From a process perspective, I don't think we should put any non-default values into the base code. It just really opens the door to code that requires changing the app for new studies, and that is not the correct route.

Actually, as I pointed out in e-mission/e-mission-docs#1007, even the current approach is not great, and we will likely never use the fallback until it is resolved, because the public dashboard does not have access to en.json

Copy link
Contributor Author

@sebastianbarry sebastianbarry Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shankari This sounds like the best standard to have moving fowrard:

  • i18next files (all supported languages): There will be basic translations for all of the default mode/purpose labels shipped with every version, for every language of the app that is supported (currently EN, ES, LO)
  • Dynamic label-options files: There will be special translations for any non-default mode/purpose labels

I have changed the requested portions of lo.json (f4d2552), so that way (for this PR) es.json and lo.json ONLY contain multi label translations for the default modes/purposes


because the public dashboard does not have access to en.json

@shankari Thank you for bringing this up! I was not considering the alternate implications changing the translation locations would be. Given this, I have a question. Currently the Lao program dynamic label-options file has "duplicated/overlapped" translations for any of the default labels (walk, e-bike, bike, etc.). Do you want me to keep these overlapped translations where they are, so that the public dashboard can still use them? I was originally thinking about going through that file and removing any overlapped translations

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't overengineer at this point, given that we are going to revisit multilabel and calculations in the future anyway.

Do you want me to keep these overlapped translations where they are, so that the public dashboard can still use them?

We will likely never use the fallback translation for dynamic labels, because the public dashboard does not have access to en.json

es/i18n/es.json Outdated
"shared_ride": "Coche de Gas, Condujo con otros",
"e_car_drove_alone": "e-coche, Condujo solo",
"e_car_shared_ride": "e-coche, Condujo con otros",
"moped": "Ciclomotor",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moped is not a standard label and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in dc888e0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants