Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add multilabel translations #47
Changes from 2 commits
48a361d
c684e20
31ee900
47b82b4
f4d2552
dc888e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
moped
is not a standard label and should be removed.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.
Removed in dc888e0
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 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.
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.
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:
lo.json
en.json
andes.json
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 fore-auto_rickshaw
andauto_rickshaw
, so any devices set to Spanish that are part of the Lao program will not have translations for those modes)e-auto_rickshaw
andauto_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, sinceen.json
is by default in the phone code ( + a change to e-mission-translate fores.json
andlo.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.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.
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:
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
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.
@shankari This sounds like the best standard to have moving fowrard:
I have changed the requested portions of
lo.json
(f4d2552), so that way (for this PR)es.json
andlo.json
ONLY contain multi label translations for the default modes/purposes@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
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 we shouldn't overengineer at this point, given that we are going to revisit multilabel and calculations in the future anyway.
We will likely never use the fallback translation for dynamic labels, because the public dashboard does not have access to en.json