-
Notifications
You must be signed in to change notification settings - Fork 2
Added localization keys to SingleNotification and AsthmaProviderReport #474
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
base: main
Are you sure you want to change the base?
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| day is noted as not under control if in the prior week (7 days) there are more than 2 days of symptoms or rescue inhaler use or due to asthma 1 | ||
| nighttime awakening or limitation in activity was recorded. | ||
| <div style={{ fontSize: '14px', color: '#3b3b3b', backgroundColor: '#f2f2f2', padding: '16px' }}> | ||
| {language('asthma-provider-report-explanation-part1')} |
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't split up prompts like this. Sentences/paragraphs won't necessarily break neatly in the same places in every language (especially RTL ones or symbolic ones). It should be possible to put the URL into the locale string directly though?
locales/sort-en.json
Outdated
| @@ -0,0 +1,594 @@ | |||
| { | |||
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.
This file should be deleted.
| <div style={{marginBottom: '16px'}}> | ||
| To provide feedback on this report or tool, please e-mail us: <a href="mailto:[email protected]" target="_blank">[email protected]</a> | ||
| <div style={{ marginBottom: '16px' }}> | ||
| {language('asthma-provider-report-feedback-part1')} |
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.
The email needs to be embedded in the prompt text. It won't necessarily appear at the end of the string in all languages. Then you can remove "part1" from the name.
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.
@nathia This one is still outstanding.
| <div style={{ position: 'absolute', bottom: 64, left: 48, right: 48, fontSize: '17px' }}> | ||
| <div style={{ fontSize: '16px', fontWeight: 700, marginBottom: '16px' }}>{language('asthma-provider-report-about-tool')}</div> | ||
| <div style={{ marginBottom: '16px' }}> | ||
| {language('asthma-provider-report-about-text-part1')} |
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.
Can't split translated string like this - embed MyDataHelps in the prompt.
|
The AsthmaProviderReport was intentionally implemented without translation because of how it is used. It is a report for the participant to share with their doctor, which, at least in its current usage, is expected to be an English speaking recipient. I think it should be okay to add these translations, but the only existing (and expected) usage of that component will need to be adjusted to pass a |
locales/de.json
Outdated
| "asthma-provider-report-about-text-part1": "Dieser Bericht wurde basierend auf den im Asthma-Tool erfassten Daten erstellt, bereitgestellt von ", | ||
| "asthma-provider-report-about-text-part2": ". Das Asthma-Tool ermöglicht es Patient, täglich ihre Symptome und Auslöser zu dokumentieren, bietet übersichtliche Zusammenfassungen der Einträge sowie relevante Bildungsinhalte und Ressourcenlinks. Es fasst die Symptomprotokolle des Nutzers zusammen. Das digitale Tool wurde mit dem Ziel entwickelt, die Selbstregulation zu fördern, was ", | ||
| "asthma-provider-report-about-text-part3": " zeigen kann, dass sich damit chronische Erkrankungen wie Asthma verbessern lassen. Das Asthma-Tool empfiehlt keine Änderungen der Behandlung und stellt keine Diagnosen.", | ||
| "asthma-provider-report-about-text": "Dieses Asthma-Management-Tool wird unterstützt von <a href=\"https://careevolution.com/mydatahelps/\" target=\"_blank\">MyDataHelps</a> und basiert auf evidenzbasierten Asthma-Management-Prinzipien aus <a href=\"https://pubmed.ncbi.nlm.nih.gov/26252889/\" target=\"_blank\">Studien</a> und anderer klinischer Forschung.", |
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'm confused - the combined about-text doesn't seem to include all of the original text from parts 1-3.
locales/en.json
Outdated
| "asthma-provider-report-day": "day", | ||
| "asthma-provider-report-days": "days", | ||
| "asthma-provider-report-disclaimer": "If you plan to share this report with your provider, be aware that communications over email, text, and other channels may not be secure because they can be sent to the wrong address or intercepted.", | ||
| "asthma-provider-report-explanation": "Asthma control is determined based on <a href=\"https://www.nhlbi.nih.gov/health-topics/guidelines-for-diagnosis-management-of-asthma\" target=\"_blank\">EPR-3 guidelines</a> and takes into account symptoms, rescue inhaler use, activity limitations, and nighttime awakenings.", |
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.
Was this change intentional? The text is quite a bit different than the original report.
| {renderStat(language('asthma-provider-report-home'), highHomeAqiDays > 0 ? highHomeAqiDays : language('asthma-provider-report-none'), highHomeAqiDays === 1 ? language('asthma-provider-report-day') : highHomeAqiDays > 1 ? language('asthma-provider-report-days') : '')} | ||
| {renderStat(language('asthma-provider-report-work'), highWorkAqiDays > 0 ? highWorkAqiDays : language('asthma-provider-report-none'), highWorkAqiDays === 1 ? language('asthma-provider-report-day') : highWorkAqiDays > 1 ? language('asthma-provider-report-days') : '')} | ||
| <div style={{ fontSize: '12px', marginTop: '12px' }}>{language('asthma-provider-report-source')}</div> | ||
| <div style={{ fontSize: '12px', marginTop: '12px' }}>Source: airnow.gov</div> |
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.
This line is a duplicate of the one above.
| <div style={{ marginBottom: '16px' }} | ||
| dangerouslySetInnerHTML={{ __html: language('asthma-provider-report-about-text') }} /> | ||
| <div style={{ marginBottom: '16px' }}> | ||
| {language('asthma-provider-report-feedback-part1')} |
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 believe the language key here should be asthma-provider-report-feedback and the link below can be removed since it will be included in the text from the language file.
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.
Yes, I forgot to stage that file.. so the change did not make it, but it should be there now.
| <div style={{flexGrow: 1, flexBasis: '30%', backgroundColor: '#f2f2f2', border: '1px solid #dbdbdb', borderRadius: '10px', padding: '12px'}}> | ||
| <div style={{marginBottom: '8px', fontWeight: 700}}>Days with AQI over 100</div> | ||
| {renderStat('Home', highHomeAqiDays > 0 ? highHomeAqiDays : 'None', highHomeAqiDays === 1 ? 'day' : highHomeAqiDays > 1 ? 'days' : '')} | ||
| {renderStat('Work', highWorkAqiDays > 0 ? highWorkAqiDays : 'None', highWorkAqiDays === 1 ? 'day' : highWorkAqiDays > 1 ? 'days' : '')} |
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.
This line should not have been removed. The duplicate is down below where indicated. Line 344, I think, where it says "Source: airnow.gov" in plain text.
| "asthma-provider-report-coughing": "Pag-ubo", | ||
| "asthma-provider-report-daily-entries": "Araw-araw na mga tala", | ||
| "asthma-provider-report-day": "araw", | ||
| "asthma-proSvider-report-days": "mga araw", |
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.
Looks like a typo here which is causing the build to fail.
Overview
The Single Notifications and Asthma Provider Report components were not using the language('key') function and had hardcoded English text, making them non-functional in other languages. I resolved this by:
DESCRIBE YOUR CHANGES
Security
DESCRIBE BRIEFLY WHAT SECURITY RISKS YOU CONSIDERED, WHY THEY DON'T APPLY, OR HOW THEY'VE BEEN MITIGATED.
Testing
SingleNotifications / No Notifications
This PR ensures that the text on the screen below now appears in the selected language.
AND in Storybook
AsthmaProviderReport Testing
DESCRIBE YOUR TEST PLAN
Documentation
@CareEvolution/api-docs.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.