Add a simple translations model #4019

eatyourgreens · 2017-09-04T14:04:13Z

Project and workflows front-end for the translations API from zooniverse/panoptes#2435.

Gets localised strings from the translations API.
Uses Redux to store the resource translations.
Updates some project and task components to use localised strings instead of API resource properties (like project.display_name.)

Review Checklist

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you rm -rf node_modules/ && npm install and app works as expected?
Did you deploy a staging branch?
If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
Are the tests passing locally and on Travis?

Optional

Have you replaced any ChangeListener or PromiseRenderer components with code that updates component state?
If changes are made to the classifier, does the dev classifier still work?
Have you added in flow type annotations?
Have you followed the Springer guidelines for commit messages?

eatyourgreens · 2017-09-04T15:12:02Z

Some links for testing:
Project default language (English): https://translation-api.pfe-preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground

Spanish translation: https://translation-api.pfe-preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground?language=es

Italian translation: https://translation-api.pfe-preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground?language=it

Greek translation (doesn't exist, falls back to English): https://translation-api.pfe-preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground?language=el

FIXED: ~~Note that requesting a translation that doesn't exist simply breaks everything.~~

eatyourgreens · 2017-09-07T13:56:06Z

Moving the localisation of the interface text into #4027

eatyourgreens · 2017-09-07T16:23:00Z

Rebased off #4027 instead of master.

eatyourgreens · 2017-09-29T11:41:22Z

Rebased to latest master.

eatyourgreens · 2017-10-05T14:16:32Z

Rebased to bring in updates to the survey task summary.

eatyourgreens · 2017-10-05T15:21:58Z

TODO: mock up some translations for the tests. Survey task tests are failing because default translations don't exist.

eatyourgreens · 2017-10-10T11:49:36Z

Following discussions on Slack yesterday, components might be loaded in from zooniverse/Zooniverse-React-Components. So, components shouldn't have to decide where strings are coming from (a translation resource or the original Panoptes resource.)

In that case, translations might be better handled in a wrapper and passed down to individual components as a prop, to avoid awkward code like translations.strings.project.title || this.props.project.display_name when rendering strings.

eatyourgreens · 2017-10-12T13:06:13Z

app/classifier/tasks/survey/translations.jsx

+import React from 'react';
+import translations from '../../../pages/project/translations';
+
+function SurveyTranslations(props) {


I find it really hard to follow what this function does 😞 (and I wrote it!)

It is really hard. I lost track at the 6th "characteristics". I know I always say this but some destructuring may help?

Any specific thoughts on how destructuring would work here? I had the same thought.

I've started to rewrite the first block and I've got this

const taskStrings = translations.strings.workflow.tasks; const { characteristics, choices, questions } = task; Object.keys(characteristics).map((characteristicId) => { translation.characteristics[characteristicId] = Object.assign({}, characteristics[characteristicId]); if (taskStrings[`${props.taskKey}.characteristics.${characteristicId}.label`]) { translation.characteristics[characteristicId].label = taskStrings[`${props.taskKey}.characteristics.${characteristicId}.label`]; } translation.characteristics[characteristicId].values = Object.assign({}, characteristics[characteristicId].values); Object.keys(task.characteristics[characteristicId].values).map((valueId) => { translation.characteristics[characteristicId].values[valueId] = Object.assign({}, characteristics[characteristicId].values[valueId]); if (taskStrings[`${props.taskKey}.characteristics.${characteristicId}.values.${valueId}.label`]) { translation.characteristics[characteristicId].values[valueId].label = taskStrings[`${props.taskKey}.characteristics.${characteristicId}.values.${valueId}.label`]; } }); });

which is slightly less convoluted, but I'd appreciate any advice you can offer to simplify it further.

The only thing I can think of is moving the second .map block out to its own method. Just to give the eyes a little rest. I'll keep thinking.

Roger wrote this for WGE at some point. I think it does what you want.

/** * Simple is object check. * @param item * @returns {boolean} */ export function isObject(item) { return (item && typeof item === 'object' && !Array.isArray(item) && item !== null); } /** * Deep merge two objects. * @param target * @param source */ export function mergeDeep(target, source) { if (isObject(target) && isObject(source)) { Object.keys(source).forEach(key => { if (isObject(source[key])) { if (!target[key]) Object.assign(target, { [key]: {} }); mergeDeep(target[key], source[key]); } else { Object.assign(target, { [key]: source[key] }); } }); } return target; }

Deep merge will get rid of all the Object.assign calls.

I'll have a think about how to reduce the number of maps. Maybe I'm looking at this the wrong way round and the code should be looping through taskStrings in order to assign translation strings to the translation object.

eatyourgreens · 2017-10-12T13:09:40Z

app/classifier/tasks/survey/translations.jsx

+
+function SurveyTranslations(props) {
+  const { task } = props;
+  const translation = {


Maybe try const translation = Object.assign({}, task) here, so that translation labels automatically default to the workflow task labels, and the following code can be greatly simplified.

I can't remember if Object.assign() does a deep merge of nested objects.

Specifically, this needs to be done in such a way that properties on translation aren't pointers to the original properties on task. 🤔

I don't think it does

The Object.assign() method only copies enumerable and own properties from a source object to a target object.

Yeah, the MDN article I linked to says that it creates references, not actual copies. We want to be able to change translation without changing task.

lodash has a deep merge method and we have it as a dependency. I think it might be used in the codebase somewhere already.

I'll have a look. I've rewritten the object cloning in cc21143 to make better use of temp variables and destructuring.

Here, we want to overwrite a key's value if a corresponding key exists in the Panoptes translation resource. The translation wrapper basically gives us something that looks like a task (but contains translated text when available) for backwards compatibility with projects that haven't been migrated over to use translations yet.

I'm also thinking that it might allow us to wrap ZRC components, and pass text into them via a translation prop. Code that doesn't use translations yet should still work by passing a task object as the translation (since the structures are identical) <Task translation={workflow.tasks[taskKey]} />.

I'm also hoping that these adapters can be ripped out, or greatly simplified, once every Panoptes resource has been migrated across to use translations.

Hm, maybe translations is a good case for a generalized higher order component in ZRC?

Possibly. I'm finding each resource needs its own, specific adapter component. There isn't much consistency between tasks, for example.

If I can figure out a generalised map from taskStrings to translation then all of this can be turned into a generic adapter for workflow task translations.

Generic translations for all resources might require some reworking of Panoptes internals. I remember @camallen saying that workflow translations have kept the awkward structure that they have here, with the long string keys for tasks, because changing those keys would break anything that relies on them, like classification exports.

eatyourgreens · 2017-10-12T13:11:22Z

app/pages/project/translations.js

+counterpart.setFallbackLocale('en');
+
+
+const translations = {


This all works, but should probably be replaced with a Redux store, and load action, at some point.

eatyourgreens · 2017-10-12T13:12:19Z

app/pages/project/home/project-home.jsx

@@ -69,7 +73,7 @@ const ProjectHomePage = (props) => {

            <div>
              <img role="presentation" src={avatarSrc} />
-              <span>&quot;{props.project.researcher_quote}&quot;</span>
+              <span>&quot;{translations.strings.project.researcher_quote || props.project.researcher_quote}&quot;</span>


This, and subsequent changes, should be updated to use the translation wrapper pattern that's being used by workflow tasks.

srallen

Thanks for clarifying what's to be expected. I only have one minor comment about the new tests and The Shortcut translations prop also is throwing a prop type warning on the test survey project when using the default English translation:
Warning: Failed prop type: The prop `translation` is marked as required in `Shortcut`, but its value is `undefined`.

srallen · 2017-11-08T16:56:03Z

app/pages/project/project-translation.spec.jsx

+};
+
+const project = {
+  id: 12345,


Not a blocker, but for accuracy with these mocks, the ids should be strings.

eatyourgreens · 2017-11-09T12:35:32Z

@srallen Thanks for taking a look. I will have a look to see why the Shortcut task is failing like that.

eatyourgreens · 2017-11-09T14:36:55Z

@srallen I can see the required prop warning, but the translation prop is defined in the Shortcut task, as far as I can tell, both in the component constructor and in componentWillMount. I'm at a loss as to how to fix that warning.

srallen · 2017-11-09T15:39:52Z

@eatyourgreens ok, it's not a blocker since it didn't seem to break anything, but it's nice to remove warning messages when we can. I'll do one last look over today.

srallen · 2017-11-10T21:52:29Z

I think this will ready to merge after a final rebase.

Adds a simple object to load and store translations. Adds translations to some project components.

Adds a general translation wrapper for all workflow tasks. Adds tests for the task translation wrapper.

Pass project translations as a separate object from project resources.

Break up some long variable names in the characteristics filter.

Translate the correct task for shortcuts. Validate shortcut answers as an array.

Move the translations object to a redux store. Add actions for loading and setting resource translations. Remove the original store object. Update ask translation tests to work with redux.

Renames project-translations for consistency.

Report resource type, ID, language and HTTP status code in API error warnings.

eatyourgreens force-pushed the translation-api branch from 483c1c5 to 8ca48ec Compare September 4, 2017 14:47

eatyourgreens force-pushed the translation-api branch 2 times, most recently from 01b002e to e4bd0f9 Compare September 6, 2017 13:56

eatyourgreens force-pushed the translation-api branch from 7f3a4cd to 910b3a8 Compare September 7, 2017 16:21

eatyourgreens changed the base branch from master to localise-projects September 7, 2017 16:22

eatyourgreens added the translations label Sep 12, 2017

eatyourgreens changed the base branch from localise-projects to master September 29, 2017 11:14

eatyourgreens force-pushed the translation-api branch from 73064c1 to beb59a0 Compare September 29, 2017 11:41

eatyourgreens force-pushed the translation-api branch from f2e4988 to dfcd0e3 Compare October 5, 2017 14:15

eatyourgreens force-pushed the translation-api branch from ecc7004 to 2ce959b Compare October 5, 2017 15:08

eatyourgreens force-pushed the translation-api branch 2 times, most recently from 852601e to c519179 Compare October 10, 2017 11:45

eatyourgreens force-pushed the translation-api branch 5 times, most recently from 7b07d49 to 03cbbf8 Compare October 12, 2017 12:12

eatyourgreens commented Oct 12, 2017

View reviewed changes

eatyourgreens force-pushed the translation-api branch 2 times, most recently from b5dec24 to 2d84685 Compare October 13, 2017 11:04

srallen suggested changes Nov 8, 2017

View reviewed changes

srallen approved these changes Nov 10, 2017

View reviewed changes

eatyourgreens added 21 commits November 14, 2017 11:49

Simple translations model

7793e6f

Adds a simple object to load and store translations. Adds translations to some project components.

Pass language as a query parameter

1c4dbc1

Default translation strings to an empty object

a81bc92

Register some Italian translations for site text

19fc995

Add a translation wrapper for projects

08223d1

Add default project and workflow translations

d49e970

Load workflow translation on workflow selection

57104e2

Update API error handling for empty responses

c8adb1c

Generic task translations

0336b41

Adds a general translation wrapper for all workflow tasks. Adds tests for the task translation wrapper.

Add a translation prop to project components

77349f6

Pass project translations as a separate object from project resources.

Tidy up the Chooser component

2a77b0d

Break up some long variable names in the characteristics filter.

Fix shortcut translations

59ef623

Translate the correct task for shortcuts. Validate shortcut answers as an array.

Add a redux store

0c018ae

Move the translations object to a redux store. Add actions for loading and setting resource translations. Remove the original store object. Update ask translation tests to work with redux.

Use redux to store current locale

d82e260

Replace 'en' with counterpart.getLocale()

c04d613

Add a test for empty workflow translations

5a6b9a5

Add tests for project translations

714b8f8

Renames project-translations for consistency.

Slightly better error warning

f655761

Report resource type, ID, language and HTTP status code in API error warnings.

Remove isRequired check for project translations

81614d3

Panoptes resource IDs are strings

d157179

Remove isRequired for Shortcut translations

1672d5d

eatyourgreens force-pushed the translation-api branch from d8a33c9 to 1672d5d Compare November 14, 2017 11:49

eatyourgreens merged commit 9ad40f9 into master Nov 14, 2017

eatyourgreens deleted the translation-api branch November 14, 2017 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple translations model #4019

Add a simple translations model #4019

eatyourgreens commented Sep 4, 2017 •

edited

Loading

eatyourgreens commented Sep 4, 2017 •

edited

Loading

eatyourgreens commented Sep 7, 2017

eatyourgreens commented Sep 7, 2017

eatyourgreens commented Sep 29, 2017

eatyourgreens commented Oct 5, 2017

eatyourgreens commented Oct 5, 2017

eatyourgreens commented Oct 10, 2017 •

edited

Loading

eatyourgreens Oct 12, 2017

simoneduca Oct 12, 2017

eatyourgreens Oct 12, 2017

eatyourgreens Oct 12, 2017

simoneduca Oct 12, 2017

simoneduca Oct 12, 2017

eatyourgreens Oct 13, 2017

eatyourgreens Oct 12, 2017

eatyourgreens Oct 12, 2017

simoneduca Oct 12, 2017

eatyourgreens Oct 12, 2017

srallen Oct 12, 2017

eatyourgreens Oct 12, 2017 •

edited

Loading

eatyourgreens Oct 12, 2017

srallen Oct 12, 2017

eatyourgreens Oct 12, 2017

eatyourgreens Oct 13, 2017

eatyourgreens Oct 12, 2017

eatyourgreens Oct 12, 2017 •

edited

Loading

srallen left a comment •

edited

Loading

srallen Nov 8, 2017

eatyourgreens commented Nov 9, 2017

eatyourgreens commented Nov 9, 2017

srallen commented Nov 9, 2017

srallen commented Nov 10, 2017

Add a simple translations model #4019

Add a simple translations model #4019

Conversation

eatyourgreens commented Sep 4, 2017 • edited Loading

Review Checklist

Optional

eatyourgreens commented Sep 4, 2017 • edited Loading

eatyourgreens commented Sep 7, 2017

eatyourgreens commented Sep 7, 2017

eatyourgreens commented Sep 29, 2017

eatyourgreens commented Oct 5, 2017

eatyourgreens commented Oct 5, 2017

eatyourgreens commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

srallen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Nov 9, 2017

eatyourgreens commented Nov 9, 2017

srallen commented Nov 9, 2017

srallen commented Nov 10, 2017

eatyourgreens commented Sep 4, 2017 •

edited

Loading

eatyourgreens commented Sep 4, 2017 •

edited

Loading

eatyourgreens commented Oct 10, 2017 •

edited

Loading

eatyourgreens Oct 12, 2017 •

edited

Loading

eatyourgreens Oct 12, 2017 •

edited

Loading

srallen left a comment •

edited

Loading