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

i18nUtils - Angular Rewrite #993

Open
sebastianbarry opened this issue Sep 29, 2023 · 7 comments
Open

i18nUtils - Angular Rewrite #993

sebastianbarry opened this issue Sep 29, 2023 · 7 comments
Assignees

Comments

@sebastianbarry
Copy link

Child issue of #977

We need to do React rewrites of all the existing angular modules; i18nUtils is only used in 2 places: Intro screen (depreciated - now we use labels) and trip_confirm_options.json

Additionally, the file only has 2 functions

  • checkFile; not used anywhere in the master branch
  • geti18nFileName

@shankari please share your thoughts about this module as it pertains to the rewrite

@shankari
Copy link
Contributor

shankari commented Sep 29, 2023

I am not sure this is needed any more. It was primarily used to load the alternate/translated files for the intro. but that was rewritten to standardize on the key/value-based translation. As you point out, the only other place where it is used is in trip_confirm.json, again to read the translation which are stored in separate files.

For that, I would suggest changing the format of trip_confirm.json to be consistent with the dynamic labels
#945
https://github.com/e-mission/nrel-openpath-deploy-configs/tree/main/label_options

There should already be code in ConfirmHelper to parse that when it is dynamic, we just need to change the "default" parse, when it is static, to also parse in the same way.

@sebastianbarry
Copy link
Author

sebastianbarry commented Oct 3, 2023

2 thoughts about this:

  1. The first purpose of the antiquated Angular i18nUtils is in confirmHelper.ts for trip_confirm_options.json
  • @JGreenlee has already implemented dynamic translations for trip confirm options built into label_options/example-program-label-options.json
  • The use of the angular i18nUtils module in this case, is for backwards compatibility (comment //backwards compat: if dynamic config doesn't have label_options, use the old way) which means that the only way the Angular i18nUtils.geti18nFileName() function will be executed is if the config loaded into the app does not have a label_options URL
  • The solution is rewriting the backwards compatibility to instead be the "default" parse behavior like you mentioned, and have it by default pull from https://github.com/e-mission/nrel-openpath-deploy-configs/blob/main/label_options/example-program-label-options.json
  • Another solution could be to replace this backwards compatibility with an error popup that forces the user to logout/ log back in to refresh the config. This will require (1) Making sure there are no config files without label_options, and (2) Adding the error popup to force users to log out/back in
  1. The second purpose of the antiquated Angular i18nUtils is in intro.js for intro/consent-text.html, intro/consent.html, and intro/summary.html
  • These files look like they can easily be internationalized, using <span ng-i18next>{{'text.here'}}</span> to input text from en.json, es.json, etc. to subsitute the hard-coded text within these HTML files. There is no obvious reason why there should be different versions of the summary and consent pages, as the only differences are textual and do not affect the format of the summary or consent pages.
  • I see that there is already infrastructure for this in en.json and es.json, where there is a currently-unused section of translations under consent-text e-mission/e-mission-phone@ce5510e written by @Abby-Wheelis which is the majority of the work for this step

Once these 2 changes have occurred, we can remove the Angular i18nUtils module

@sebastianbarry sebastianbarry self-assigned this Oct 3, 2023
@JGreenlee
Copy link

I agree that we should get rid of i18nUtils.

I believe (2) is already done. The new onboarding flow (e-mission/e-mission-phone#1032, merged last night) doesn't use any internationalized filenames.

For (1), I am not sure if forcing every study/program onto the dynamic label options is the right move. I think we should still have a local fallback if label_options is not present in the dynamic config. This way we don't disrupt any ongoing program/studies.

The local fallback can use the same format as the https://github.com/e-mission/nrel-openpath-deploy-configs/tree/main/label_options. In doing so, we won't need internationalized filenames and can remove i18nUtils

@sebastianbarry
Copy link
Author

Per @JGreenlee, I will base these changes on onboarding_routing_sept_2023 and PR to service_rewrite_2023

@sebastianbarry
Copy link
Author

sebastianbarry commented Oct 3, 2023

The local fallback can use the same format as the https://github.com/e-mission/nrel-openpath-deploy-configs/tree/main/label_options. In doing so, we won't need internationalized filenames and can remove i18nUtils

@JGreenlee and @Abby-Wheelis concerning the design, which do you think would make more sense to have the local fallback (for studies/programs not using label_options dynamic labels) resort to?

OR

  • (opt. 2) Create 2 new default-program-label-options.json and default-study-label-options.json in nrel-openpath-deploy-configs/label_options that function the same way to example-program-label-options.json and example-study-label-options.json

OR

  • Create ONE new default-label-options.json

OR

  • (opt. 3) (I don't think this is the best option, but I think it is at least worth putting for the record...) A local default-label-options.json file, so that we don't need to fetch for an online location

I like the (opt. 2) or (opt. 3) the most, because the word default communicates that it is the default/fallback option, whereas example communicates that it is some sort of template. As far as (opt. 2) vs. (opt. 3), they look identical so I don't see a reason why opt. 2 would be better than opt. 3

@JGreenlee
Copy link

I think Opt. 3 is what was requested a few days ago (at least by my interpretation):

There should already be code in ConfirmHelper to parse that when it is dynamic, we just need to change the "default" parse, when it is static, to also parse in the same way.

For program/studies that don't want to use dynamic label options and just want to use our defaults, (which may be the majority of program/studies), why implicate an extra network call if we don't need to?
I think a local fallback of default-label-options.json, that is actually included in the phone code and doesn't need to be fetched, is just simpler and more foolproof.

@sebastianbarry
Copy link
Author

sebastianbarry commented Oct 5, 2023

PR Submitted: e-mission/e-mission-phone#1055 with testing!

@JGreenlee and @shankari I asked 2 questions in there, could you review and let me know how you think would be best to proceed?

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

No branches or pull requests

3 participants