-
-
Notifications
You must be signed in to change notification settings - Fork 55
[WEB-279] Feature/spanish #699
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
[WEB-279] Feature/spanish #699
Conversation
…eature/spanish-version
clintonium-119
left a comment
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.
Thanks for sharing this back with us.
I have a couple small changes requested -- mostly to do with fixing the automated tests.
Just ping me for re-review when you've implemented them, and I'll get this merged. I am going to need to check with the appropriate team members for whether or not I can merge in community-provided translations, but for now, let's leave them here.
app/pages/patientdata/patientdata.js
Outdated
| const { Loader } = vizComponents; | ||
| const { findBasicsStart, getLocalizedCeiling, getTimezoneFromTimePrefs } = vizUtils.datetime; | ||
| const { commonStats, getStatDefinition } = vizUtils.stat; | ||
| const t = i18next.t.bind(i18next); |
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 can be removed here, since the t prop is being added in to this React class automatically.
app/pages/patientdata/patientdata.js
Outdated
| }, | ||
|
|
||
| renderEmptyHeader: function(title = 'Preparing Chart Data') { | ||
| renderEmptyHeader: function(title = t('Preparing Chart Data')) { |
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.
Using t in the function signature here solves the issue you were addressing, but is causing the automated lint test to fail.
I suggest setting the default title within the function body. Something like:
renderEmptyHeader: function(title) {
const { t } = this.props;
const headerTitle = title || t('Preparing Chart Data');
return (
<Header
chartType={'no-data'}
inTransition={false}
atMostRecent={false}
title={headerTitle}
ref="header"
/>
);
},
i18next-parser.config.json
Outdated
| }, | ||
| "sort": true, | ||
| "locales": ["en", "fr"] | ||
| "locales": ["en", "fr","es"] |
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.
Tiny nitpick -- should have a space after the comma for consistency.
|
One other thing: we'll need you to agree to our VCLA with a comment here before I can merge as well. The details can be found at https://developer.tidepool.org/contributors/#making-contributions |
|
I agree to the terms of Tidepool Project’s Volunteer/Contributor License Agreement v1.1 as it exists at http://tidepool-org.github.io/files/TidepoolVCLA-1.1.pdf on 06/april/2020 and I sent mail to account legal@tidepool.org. |
Part of WEB-279
Creation of a Spanish version for the files. Spanish language was added in the i18next configuration.
I upload key translations (spanish), I don't know if it's necessary.