Allows configuring available languages through env and adds Hindi to CARE #8692
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
148a154
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5bbaeef8.care-fe.pages.dev |
| Branch Preview URL: | https://add-hindi.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
add-hindi
|
| Run status |
|
| Run duration | 03m 11s |
| Commit |
|
| Committer | Shivank Kacker |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
124
|
| View all changes introduced in this branch ↗︎ | |
.env
Outdated
| @@ -12,3 +12,4 @@ REACT_CARE_API_URL=https://careapi.ohc.network | |||
| # Dev envs | |||
| ESLINT_NO_DEV_ERRORS=true | |||
| CARE_CDN_URL="https://egov-s3-facility-10bedicu.s3.amazonaws.com https://egov-s3-patient-data-10bedicu.s3.amazonaws.com http://localhost:4566" | |||
| REACT_AVAILABLE_LOCALES="EN,HI,TA,ML,MR,KN" | |||
There was a problem hiding this comment.
wouldn't 'ALLOWED_LOCALES' term be more appropriate?
care.config.ts
Outdated
| @@ -38,6 +38,9 @@ const careConfig = { | |||
| customLogo: logo(env.REACT_CUSTOM_LOGO), | |||
| customLogoAlt: logo(env.REACT_CUSTOM_LOGO_ALT), | |||
| customDescription: env.REACT_CUSTOM_DESCRIPTION, | |||
| availableLocales: (env.REACT_AVAILABLE_LOCALES || "") | |||
| .split(",") | |||
| .map((l) => l.trim().toLowerCase()), | |||
There was a problem hiding this comment.
| .map((l) => l.trim().toLowerCase()), | |
| .map((l) => l.trim()), |
There was a problem hiding this comment.
@bodhish this is to be sure that random case characters in the ENV don't break things.
There was a problem hiding this comment.
Lets not have it, env configuration is one time and validation for the env will take care of it. #8470
src/Locale/hi/index.js
Outdated
| @@ -0,0 +1,45 @@ | |||
| import Asset from "./Asset.json"; | |||
There was a problem hiding this comment.
Do we need this for all languages 🤔
Can't we write a dynamic import? ie all files in a folder 🤔
There was a problem hiding this comment.
Updated. Using dynamic imports as much as possible now.
There was a problem hiding this comment.
Update: Dynamic imports were causing issues during build due to their async nature. Reverted back.
There was a problem hiding this comment.
I really don't want us to have this file for each language, not a priority to fix now but I think we should find a better solution for this.
There was a problem hiding this comment.
We can take this up in a later issue. Tracking it through #8705
| const availableLocales = Object.keys(LANGUAGE_NAMES).filter( | ||
| (l) => l === "en" || careConfig.availableLocales?.includes(l), | ||
| ); |
There was a problem hiding this comment.
why do we need a special case for en?
There was a problem hiding this comment.
So that even when there are no available languages set in env english is still there
There was a problem hiding this comment.
I think its safe to assume that the env will configured correctly, we also have a default set, so I think we can ignore it
|
@shivankacker iam seeing commits and conversation going on, is it good for testing |
shivankacker
added
needs testing
and removed
question
|
@nihal467 this is ready for testing |
|
LGTM |
|
@shivankacker @shivankacker Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
availableLocalesvariable in care config, derived from theREACT_AVAILABLE_LOCALESvariable.@ohcnetwork/care-fe-code-reviewers
Merge Checklist