Generalise the contents editor #5

eatyourgreens · 2017-05-31T14:03:43Z

No description provided.

simoneduca · 2017-05-31T15:58:26Z

src/ducks/contents.js

@@ -26,13 +26,16 @@ const projectContentsReducer = (state = initialState, action) => {
 };

 // Action Creators
-const fetchProjectContents = (project_id) => {
+const fetchProjectContents = (id, type) => {
+  type = type ? type.split('/')[0] : 'project';
  return (dispatch) => {
    dispatch({
      type: FETCH_PROJECT_CONTENTS,


Action types should also be generalised.

Good point.

I was thinking that this should be a generic loader for any of the resources listed in #6, so that new resource types can be added without having to write a lot of new code.

Maybe we should define a TranslationResource component (I'm not sure about the name), which is a generic adapter for specific translations, something like the PFE file viewer.

eatyourgreens · 2017-06-06T13:41:00Z

src/ducks/projects.js

@@ -32,7 +32,8 @@ const fetchProjects = () => {
      type: FETCH_PROJECTS,
    });
    const query = {
-      current_user_roles: ['owner', 'translator']
+      current_user_roles: ['owner', 'translator'],
+      include: ['workflows']


This query fetches all projects, so including all workflows is probably a bad idea.

Can we filter workflows? I can't think of a way involving not fetching all projects.

I'm thinking just don't include them here. They aren't needed until you select a project to work on.

Projects like NfN and Wildcam Gorongosa, with large numbers of workflows, would still be a problem.

simoneduca · 2017-06-06T14:30:52Z

src/containers/ProjectContentsContainer.jsx

+  children: PropTypes.node,
+  contents: PropTypes.object,
+  params: PropTypes.shape({
+    project_id: PropTypes.string


Missing resource_id: PropTypes.string

simoneduca · 2017-06-06T15:19:43Z

src/ducks/resource.js

+      title: contents.title,
+      description: contents.description,
+      introduction: contents.introduction,
+      language: 'nz',


It's fine for testing, but this will have to be a parameter, rather than hardcoded.

eatyourgreens · 2017-06-06T15:26:50Z

src/ducks/resource.js

+    const key = `${type}_id`;
+    const query = {};
+    query[key] = id;
+    apiClient.type(`${type}_contents`).get(query)


type here only works for workflows and projects. Tutorials and field guides will have a different rule for determining resource type.

eatyourgreens · 2017-06-06T16:08:53Z

src/ducks/resource.js

+};
+
+// Action Creators
+const fetchResource = (id, type) => {


This function should also take language as a parameter.

eatyourgreens · 2017-06-06T16:09:24Z

src/ducks/resource.js

+      type: FETCH_RESOURCE,
+    });
+    const key = `${type}_id`;
+    const query = {};


query should specify the resource language, I think.

eatyourgreens · 2017-06-13T16:23:35Z

Tests failing because yarn won't install. I'm not sure why Travis is installing yarn.

srallen · 2017-06-13T16:36:41Z

It'll try to install yarn if there's a yarn.lock file in the root of the repository. Why yarn fails to install on travis, I'm not sure yet since we were running into that with the lab app. I'm thinking of scrapping yarn for npm 5.

eatyourgreens · 2017-06-14T13:42:48Z

src/containers/ProjectContentsContainer.jsx

-    return actions.fetchProjectContents(this.props.params.project_id);
+    const type = this.props.params.resource_type;
+    const id = type ? this.props.params.resource_id : this.props.params.project_id;
+    return actions.fetchResource(id, type);


Why are we returning a value from componentDidMount?

Sorry, just some cruft. However, I have experienced problems in triggering actions when return weren't used...

eatyourgreens · 2017-06-15T11:52:01Z

src/ducks/resource.js

+      description: contents.description,
+      introduction: contents.introduction,
+      language: 'nz',
+      'links.project': contents.links.project,


This will need generalising too. Resources aren't necessarily linked to the project eg. Tutorials and WorkflowContents are linked to workflows.

Adds workflow contents, by ID. Lists all workflow strings.

Generalise resource loading and display. Add a bit of a hack to handle ProjectContents and WorkflowContents.

simoneduca · 2017-06-21T10:57:20Z

src/components/ProjectDashboard.jsx

+      </ProjectContentsContainer>
+      <h3>Workflows</h3>
+      <ul>
+        {project.workflows.map((workflow) => {


This throws if workflows is undefined.

simoneduca · 2017-06-21T10:57:36Z

src/components/ProjectDashboard.jsx

+      </ul>
+      <h3>Tutorials</h3>
+      <ul>
+        {project.tutorials.map((tutorial) => {


This throws if tutorials is undefined.

Should be fixed now.

Default workflows and tutorials to empty arrays. Store them in project state, not the project resource.

simoneduca · 2017-06-21T14:35:53Z

src/ducks/project.js

+    };
+    apiClient.type('projects').get(query)
+    .then(([project]) => {
+      dispatch(fetchWorkflows(project));


I think dispatching multiple actions within a dispatch call isn't recommended. That might lead to unnecessary calls to store.dispatch. There are ways for batching actions I'm reading up about.
In general I think having actions dispatched from components improves understanding of what is going on, instead of having to follow all the nested calls inside the action creators.

Actually, we're using a middleware, whose point is to accommodate dispatching actions with actions. Only the readability concern remains, but no biggie.

simoneduca · 2017-06-21T14:54:24Z

src/ducks/resource.js

+    case FETCH_RESOURCE:
+      return Object.assign({}, initialState, { loading: true });
+    case FETCH_RESOURCE_SUCCESS:
+      return Object.assign({}, state, { original: action.payload, loading: false });


What is original supposed to store? Given the name, I was expecting to see in here only the original in english and any available translation under translation. However I see allI the clones with different language flags I created in some early tests. I'm a bit confused.

Yeah, my aim is to eventually store the original here, maybe split the translations out into their own array.

At the moment, the API query will bring back everything. We can either update the query so that it only fetches the original resource, or split the response into the original + translations.

translation, by the way, is the translation that you're currently working on, so a single resource.

That makes sense. Cheers

Add type validation for URL params. Don't return a value from ComponentDidMount.

eatyourgreens force-pushed the general-editor branch 2 times, most recently from db9cc26 to a3d9ed7 Compare May 31, 2017 15:18

simoneduca reviewed May 31, 2017

View reviewed changes

eatyourgreens commented Jun 6, 2017

View reviewed changes

simoneduca reviewed Jun 6, 2017

View reviewed changes

eatyourgreens force-pushed the general-editor branch from 26eabbe to cc3440f Compare June 6, 2017 15:20

eatyourgreens commented Jun 6, 2017

View reviewed changes

eatyourgreens force-pushed the general-editor branch 2 times, most recently from b49dfdd to f294e03 Compare June 13, 2017 14:50

eatyourgreens changed the title ~~[WIP] Generalise the contents editor~~ Generalise the contents editor Jun 13, 2017

eatyourgreens force-pushed the general-editor branch from 808c8da to 1733091 Compare June 14, 2017 13:40

eatyourgreens commented Jun 14, 2017

View reviewed changes

eatyourgreens mentioned this pull request Jun 15, 2017

Use cases #11

Open

18 tasks

eatyourgreens commented Jun 15, 2017

View reviewed changes

eatyourgreens force-pushed the general-editor branch from 1733091 to 9a56746 Compare June 15, 2017 13:50

eatyourgreens added 8 commits June 21, 2017 11:52

Extract ProjectContents from editor

002de34

Link up workflow contents

f17ca33

Adds workflow contents, by ID. Lists all workflow strings.

Add actions for generic Panoptes resources

365665d

Remove workflows from project list

ed87a3e

Add a rough project dashboard

4b15a09

Load workflows with the project

7b57687

Add tutorials as generic resources.

dfb081d

Generalise resource loading and display. Add a bit of a hack to handle ProjectContents and WorkflowContents.

Add an action to create new translations

2303339

eatyourgreens force-pushed the general-editor branch from f15422f to 2303339 Compare June 21, 2017 10:52

simoneduca reviewed Jun 21, 2017

View reviewed changes

Set appropriate defaults for workflows and tutorials

d04baab

Default workflows and tutorials to empty arrays. Store them in project state, not the project resource.

simoneduca reviewed Jun 21, 2017

View reviewed changes

Fix some linter warnings

7ef41fb

Add type validation for URL params. Don't return a value from ComponentDidMount.

eatyourgreens force-pushed the general-editor branch from c68203c to 7ef41fb Compare June 22, 2017 09:24

eatyourgreens merged commit c160b51 into master Jun 22, 2017

eatyourgreens deleted the general-editor branch June 22, 2017 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalise the contents editor #5

Generalise the contents editor #5

eatyourgreens commented May 31, 2017

simoneduca May 31, 2017

eatyourgreens Jun 1, 2017

eatyourgreens Jun 6, 2017

simoneduca Jun 6, 2017

eatyourgreens Jun 6, 2017

simoneduca Jun 6, 2017

simoneduca Jun 6, 2017 •

edited

Loading

eatyourgreens Jun 6, 2017

eatyourgreens Jun 6, 2017

eatyourgreens Jun 6, 2017

eatyourgreens commented Jun 13, 2017

srallen commented Jun 13, 2017

eatyourgreens Jun 14, 2017

simoneduca Jun 16, 2017

eatyourgreens Jun 15, 2017

simoneduca Jun 21, 2017

simoneduca Jun 21, 2017

eatyourgreens Jun 21, 2017

simoneduca Jun 21, 2017

simoneduca Jun 21, 2017

simoneduca Jun 21, 2017

simoneduca Jun 21, 2017 •

edited

Loading

eatyourgreens Jun 22, 2017

simoneduca Jun 22, 2017

Generalise the contents editor #5

Generalise the contents editor #5

Conversation

eatyourgreens commented May 31, 2017

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

simoneduca Jun 6, 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

eatyourgreens commented Jun 13, 2017

srallen commented Jun 13, 2017

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

simoneduca Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneduca Jun 6, 2017 •

edited

Loading

simoneduca Jun 21, 2017 •

edited

Loading