🔖 [PR] New default label options and translations #1065

sebastianbarry · 2023-10-12T19:22:30Z

This PR is associated with e-mission/e-mission-docs#1007

This PR is replacing #1055, as that branch was having issues loading the app that I wasn't sure how to troubleshoot (likely something new changed since oct5 - oct9 commits from @JGreenlee and @shankari):

This new branch works, and includes everything from the old PR but adds the function of falling back to checking i18next translations in a new field named "multilabel" for label-options if the translations are not defined in the provided dynamic (or default, technically) label-options file.

confirmHelper.ts - Removed archaic behavior or using old Angular i18nUtils module to get the filename of the translations' trip_confirm_options.json file - The "else" default "no label_options found in config" behavior is almost identical to the if(appConfig.label_options), but it just uses the default label options URL at label-options.json.sample - Pulled the language-specific text handling behavior out of the if statement, because either way (label_options or no label_options) the JSON data model will look the same - The for loop for filling in the translatied text for label-options checks 1. The label-options file first, then 2. The i18next file for the translation label-options.json.sample - Created new file - Replaces trip_confirm_options.json.sample (this location in confirmHelper.ts was the only place in the codebase using this file) - Modeled after https://github.com/e-mission/nrel-openpath-deploy-configs/blob/main/label_options/example-program-label-options.json - Only has translations for EN and ES trip_confirm_options.json.sample - Removed this file

en.json - Added multilabel filed filled with a few multilabel translations in EN

sebastianbarry · 2023-10-12T19:46:21Z

Testing:

Scenario	App Screenshot	Code screenshot
I added a new `"example"` MODE which gathers its translation from label-options.json.sample by default
I removed the translation from label-options.json.sample and it fell-back onto the translation I added to en.json

sebastianbarry · 2023-10-12T19:48:13Z

Ready for review

JGreenlee · 2023-10-13T04:57:40Z

This PR cannot be merged until service_rewrite_2023 is in sync with onboarding_routing_sept_2023.
Once synced, we'll see if any of these merge conflicts are still there - if so we'll have to resolve them

JGreenlee

The logic looks good. I am requesting a few textual changes.

We are going to need a PR to e-mission-translate adding the new keys to es.json and lo.json. Feel free to either do that yourself or coordinate with @Abby-Wheelis since she's been handling translations recently.

www/js/survey/multilabel/confirmHelper.ts

www/i18n/en.json

JGreenlee · 2023-10-16T16:40:35Z

Can you change the target of this PR to some other branch, then change it back to service_rewrite_2023?

(My PRs are merged but my commits are still showing up in this PR. I've discovered that toggling the branch forces Github to update the diff)

www/js/survey/multilabel/confirmHelper.ts

> JGreenlee > It's the other way around. labelOptions first, then i18next. > The code is correct but the comment is not

> JGreenlee > While we are rewriting and have this opportunity, can we make all the text with slashes look "like / this" instead of "like/ this"? > I believe they were made like that a while ago to force a line break. But we don't do that anymore in the UI so the slashes end up looking weird.

…github.com/sebastianbarry/e-mission-phone into new-default-label-options-and-translations

sebastianbarry · 2023-10-17T17:17:23Z

Last thing left to do is:

PR to e-mission-translate adding the new keys to es.json and lo.json

JGreenlee

LGTM

sebastianbarry · 2023-10-17T17:49:25Z

Created PR to e-mission-translate e-mission/e-mission-translate#47

shankari

@sebastianbarry this is a cool idea, but I don't understand why you need to have two copies of the translations in this case. If the whole point was to have one "source of truth" and not copy the information around, then there's no reason to have two copies...

Also, where is the testing for default versus dynamic config, which was the original issue?
This only tests for default translations from label-options and from en.json...

www/js/survey/multilabel/confirmHelper.ts

www/json/label-options.json.sample

www/i18n/en.json

sebastianbarry · 2023-10-19T15:27:06Z

this is a cool idea, but I don't understand why you need to have two copies of the translations in this case. If the whole point was to have one "source of truth" and not copy the information around, then there's no reason to have two copies...

The design principle for having 2 copies of the translations is as follows: There is an underlying layer of multilabel translations, found in the i18next files en.json, es.json, etc. These translations are the fallback in case any label-options file does not contain the translations portion for a given language. Then above that layer, are the translations from the label-options file. These translations will be the default. Having this default/backup translations is helpful for multiple reasons: 1. If there is a Spanish speaking user who is part of the Lao program that only has translations in Lao and English, they prefer Spanish on their device which does not have translations in the Lao label-options file. In this case, it would fall back onto the es.json file and get the translations from there. 2. In USA English we might call a mode of transportation a "Rickshaw", but in Lao English they might call a mode of transportation "Tuk tuk", so we have the ability to have area-specific translations of modes.

This method of implementation does have some (but minimal) copied translations, but it offers a higher level of flexibility because the label-options file only needs to have special translations, and any other way of doing it would not offer this level of flexibility with this level of customization.

That being said @shankari, I see what you're saying about this PR still having translations in the default sample label-options file, so I will remove those.

Also, where is the testing for default versus dynamic config, which was the original issue? This only tests for default translations from label-options and from en.json...

The testing for default vs. dynamic config was in the old PR that I closed:

Testing:

	iPhone 13 iOS 15.4
Labels being loaded locally
Labels working
Labels working

Since the default translations are in 18next files, we don't need a duplicate of the default label options translations in this file Also changed the logic because I didn't have it right when labelOptions.translations is undefined

sebastianbarry · 2023-10-20T19:14:17Z

I removed the translations from label-options.json.sample and confirmed that it is still working - it required a change to the logic as well to get the translation, but it works for various purposes now

label-options has NO translations (gets every translation from i18next)
label-options has only some translations (first checks label-options, then checks i18next)
label-options has all the translations (only checks label-options, but still has the backup of i18next in case one is missing or something)

@shankari Moving this back to ready for review

shankari · 2023-10-20T23:08:10Z

This method of implementation does have some (but minimal) copied translations, but it offers a higher level of flexibility because the label-options file only needs to have special translations, and any other way of doing it would not offer this level of flexibility with this level of customization.

Correct, but before your most recent change, the label-options file had all translations, not just special ones.

sebastianbarry · 2023-10-20T23:58:27Z

This method of implementation does have some (but minimal) copied translations, but it offers a higher level of flexibility because the label-options file only needs to have special translations, and any other way of doing it would not offer this level of flexibility with this level of customization.

Correct, but before your most recent change, the label-options file had all translations, not just special ones.

I added some of my thoughts to this in e-mission/e-mission-translate#47 (comment), adding it here for traction

sebastianbarry added 2 commits October 12, 2023 13:16

Adding examples of multilabel translations to en.json

6508517

en.json - Added multilabel filed filled with a few multilabel translations in EN

This was referenced Oct 12, 2023

Deault label options #1055

Closed

🔖 Re-design of Modes/Purpose Multilabel Translations e-mission/e-mission-docs#1007

Open

sebastianbarry changed the title ~~New default label options and translations~~ 🔖 [PR] New default label options and translations Oct 12, 2023

JGreenlee requested changes Oct 13, 2023

View reviewed changes

www/js/survey/multilabel/confirmHelper.ts Outdated Show resolved Hide resolved

www/i18n/en.json Outdated Show resolved Hide resolved

shankari mentioned this pull request Oct 13, 2023

Pulling from onboarding to keep branches in sync #1066

Merged

sebastianbarry changed the base branch from service_rewrite_2023 to master October 17, 2023 17:03

sebastianbarry changed the base branch from master to service_rewrite_2023 October 17, 2023 17:03

sebastianbarry commented Oct 17, 2023

View reviewed changes

www/js/survey/multilabel/confirmHelper.ts Outdated Show resolved Hide resolved

sebastianbarry and others added 3 commits October 17, 2023 11:07

Correct text in comment

36b56da

> JGreenlee > It's the other way around. labelOptions first, then i18next. > The code is correct but the comment is not

Merge branch 'new-default-label-options-and-translations' of https://…

1bb9055

…github.com/sebastianbarry/e-mission-phone into new-default-label-options-and-translations

JGreenlee approved these changes Oct 17, 2023

View reviewed changes

sebastianbarry mentioned this pull request Oct 17, 2023

Add multilabel translations e-mission/e-mission-translate#47

Merged

shankari requested changes Oct 19, 2023

View reviewed changes

www/js/survey/multilabel/confirmHelper.ts Outdated Show resolved Hide resolved

www/json/label-options.json.sample Outdated Show resolved Hide resolved

www/i18n/en.json Show resolved Hide resolved

Remove translations from label-options.json.sample

2c6739a

Since the default translations are in 18next files, we don't need a duplicate of the default label options translations in this file Also changed the logic because I didn't have it right when labelOptions.translations is undefined

shankari merged commit 2e19760 into e-mission:service_rewrite_2023 Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔖 [PR] New default label options and translations #1065

🔖 [PR] New default label options and translations #1065

sebastianbarry commented Oct 12, 2023

sebastianbarry commented Oct 12, 2023 •

edited

Loading

sebastianbarry commented Oct 12, 2023

JGreenlee commented Oct 13, 2023 •

edited

Loading

JGreenlee left a comment

JGreenlee commented Oct 16, 2023 •

edited

Loading

sebastianbarry commented Oct 17, 2023 •

edited

Loading

JGreenlee left a comment

sebastianbarry commented Oct 17, 2023

shankari left a comment •

edited

Loading

sebastianbarry commented Oct 19, 2023

sebastianbarry commented Oct 20, 2023

shankari commented Oct 20, 2023

sebastianbarry commented Oct 20, 2023

🔖 [PR] New default label options and translations #1065

🔖 [PR] New default label options and translations #1065

Conversation

sebastianbarry commented Oct 12, 2023

sebastianbarry commented Oct 12, 2023 • edited Loading

sebastianbarry commented Oct 12, 2023

JGreenlee commented Oct 13, 2023 • edited Loading

JGreenlee left a comment

Choose a reason for hiding this comment

JGreenlee commented Oct 16, 2023 • edited Loading

sebastianbarry commented Oct 17, 2023 • edited Loading

JGreenlee left a comment

Choose a reason for hiding this comment

sebastianbarry commented Oct 17, 2023

shankari left a comment • edited Loading

Choose a reason for hiding this comment

sebastianbarry commented Oct 19, 2023

sebastianbarry commented Oct 20, 2023

shankari commented Oct 20, 2023

sebastianbarry commented Oct 20, 2023

sebastianbarry commented Oct 12, 2023 •

edited

Loading

JGreenlee commented Oct 13, 2023 •

edited

Loading

JGreenlee commented Oct 16, 2023 •

edited

Loading

sebastianbarry commented Oct 17, 2023 •

edited

Loading

shankari left a comment •

edited

Loading