Create new Questionnaire (integration) #140

hamishtaplin · 2017-06-14T10:40:59Z

Changes

Added clientside logic for connecting to Author API:

Integrated Apollo Client
Removed questionnaire meta from Redux
Moved update logic to form input/textarea/select components
Connected frontend to Apollo backend
Updated breadcrumbs to work with backend
Added tests

Testing

Check process of adding a Questionnaire works and is stored in backend

bennpearson · 2017-06-20T08:42:05Z

src/pages/CreateQuestionnaire.js

-  onChange: PropTypes.func
+  history: PropTypes.object.isRequired, // eslint-disable-line
+  loading: PropTypes.bool.isRequired,
+  onUpdate: PropTypes.func.isRequired,


do we need error propType here?

@bennpearson currently not dealing with errors, probably need to at some point though: https://trello.com/c/oQ5Nn9ll/88-error-handling

WickyNilliams

Looks good, couple of small points

WickyNilliams · 2017-06-21T12:29:59Z

src/components/Forms/Input/index.js

  type: PropTypes.oneOf(["text", "checkbox", "radio"]).isRequired,
-  value: PropTypes.oneOfType([
+  defaultValue: PropTypes.oneOfType([


why defaultValue is you're now using onChange handlers?

If we use value then you're depending on a (debounced) roundtrip to the api to update the value—makes for a rather laggy typing experience.

Using defaultValue sets the value on initial render but then the state of the value is handled internally, with updates being periodically sent to the API.

We shouldn't be saving as a user types. The value of the form shouldn't be tied to a request to the server. It should go into the store, then pulled out when necessary.
In any case, saving as a user types opens up corner cases like rollback happening as user types - would actually cause them to lose work!

I personally think we should do auto-saving as a separate story rather than ad-hoc, as that tends to result in a better solution (e.g. perhaps it can be done cleanly with middleware?)

Can we address this as a separate "saving/auto-saving" story when we have some more UI to work with?

I think that's wise. How about changing to autosave on blur for now? Conceptually simpler and less corner cases

WickyNilliams · 2017-06-21T12:30:49Z

src/containers/Breadcrumb.js

-  const route = getRouteByPath(pathname);
+const getQuestionnaire = gql`
+  query GetQuestionnaire {
+    questionnaire(id: 1) {


this should be paramterised

I paramterised the gql on Publisher, see here for an example.

WickyNilliams · 2017-06-21T12:31:44Z

src/containers/Breadcrumb.js

-  const route = getRouteByPath(pathname);
+const getQuestionnaire = gql`
+  query GetQuestionnaire {
+    questionnaire(id: 1) {


Why is the breadcrumb requesting this data? Seems like an odd place for the request to be made. Especially when CreateQuestionnaire seems to do the same. Seems like the type of thing that should flow down through props

I wasn't entirely happy with this but the alternative is a significant refactor. Structurally, CreateQuestionnaire and the Breadcrumbs are in different timezones, the data fetching would have to be done at layout level and then the props passed down through several layers of components, which comes with its own problems.

Might be worth revisiting the architecture at some point. Particularly at which "level" we use container components. The Layout/Page hierarchy was something I threw together to get it working initially. If it is causing problems then we can revisit it. But it's probably outside the scope of this PR.

The value goes into the store, presumably? So couldn't Breadcrumb be connected to store and pull the value out of there?

@samiwel I think we should refactor if it becomes an issue. At the moment it's not causing any problems.

Hmm yeah. But that was reading directly from the store and being dependent on the keying strategy used. Though there are methods for pulling stuff from the store without hitting the server: http://dev.apollodata.com/core/read-and-write.html. readQuery or readFragment are what we want, I think

Wasn't aware of that, I'll look into it. However, does't that rely on the request having been made elsewhere? Eg. if you navigate straight to the /design page there's no request currently so the breadcrumbs won't have a clue what's going on.

Yes it would rely on that. Haha, guess this goes back to the id in the url issue. OK let's leave for now. Do you want to add some tasks to trello to cover all the stuff we're going to defer for now?

Looking ahead—the Header/Nav will probably need to be more connected. I think that will present opportunities to connect higher up and pass things like title down to Breadcrumbs.

That sounds sensible. I wouldn't be surprised if it eventually ends up connected at page level, with everything flowing down from there

WickyNilliams · 2017-06-21T12:33:20Z

src/containers/CreateQuestionnaire.js

+      navigation,
+      legalBasis,
+      theme,
+      __typename


I don't think you need this in your query. Is there a reason for it?

This was added whilst attempting to get MockedProvider to work in the tests. See: apollographql/react-apollo#674 (comment)

Did it make it work? If not, can it be removed?

Ignore, I see you removed it

WickyNilliams · 2017-06-21T12:35:06Z

src/containers/CreateQuestionnaire.js

+`;
+
+const mapResultsToProps = results => {
+  const { data: { loading, questionnaire } } = results;


if you're going to do deep destructuring, what do you think about formatting it so it mirrors the structure of the object? e.g.

const { data : { loading, questionnaire } } = results;

alternatively:

const { loading, questionnaire } = results.data;

Prefer the latter 👍

WickyNilliams · 2017-06-21T12:35:09Z

src/containers/CreateQuestionnaire.js

+
+const mapResultsToProps = results => {
+  const { data: { loading, questionnaire } } = results;
+  // console.log(results.data.error);


remove pls :)

WickyNilliams · 2017-06-21T12:35:38Z

src/containers/CreateQuestionnaire.js

-);
+export const withData = graphql(getQuestionnaire, {
+  props: mapResultsToProps,
+  options: { variables: { id: 1 } }


shouldn't have the id hard-coded

Where should this live? We have to hardcode it somewhere, I think as we're only loading one questionnaire.

Can you create a function called withData which accepts a parameter (the Id) and returns the graphql? e.g. something like:

const withData = questionnaireId => { return graphql(getQuestionnaire, { props: mapResultsToProps, options: { variables: { id: questionnaireId } } }); }

Then call with withData(1)

The id should come from the url i guess? When the questionnaire obj is in memory, you should have its id available

@samiwel This just moves the hardcoded value elsewhere?

@WickyNilliams That would work but in the interests of moving PRs on a bit quicker, I think it's better that we solve problems (such as being able to dynamically select a questionnaire) when we need that feature.

The URL stuff we could do later as that's a bigger task. But for now, you could pass the id back from the component?

That's still just hardcoding the value elsewhere, solving a problem we don't have yet. We likely won't be passing that id in from the component in future so it will require another refactor further down the line.

I mean it should receive as prop and pass back? Or the callback could grab the value from somewhere before calling mutate. Either way, I absolutely think we be shouldn't be hardcoding values now

WickyNilliams · 2017-06-21T12:38:05Z

src/pages/CreateQuestionnaire.js

+export class CreateQuestionnairePage extends Component {
+  constructor(props) {
+    super(props);
+    this.debouncedChangeHandler = debounce(this.onChange.bind(this), 300, {


surprised this needs binding if you're using a property initializer below, since that should already be bound to this

I was also surprised but it doesn't work otherwise, I think related to this answer on SO.

Hmm, that answer suggested this should work:

onChange = debounce(value => { this.setState(value, () => { this.props.onUpdate(this.state); }); }, 300, { leading: true });

@WickyNilliams that was the first thing I did, it doesn't work. :(

Hmm, screw it then. Leave as-is for now!

Moving to update on blur just made this irrelevant anyway.

All part of my master plan

WickyNilliams · 2017-06-21T12:39:02Z

src/pages/CreateQuestionnaire.js

-          </ActionButtonGroup>
+          {loading
+            ? <div>Loading...</div>
+            : <Form handleSubmit={this.onSubmit}>


Can you extract the two branches into methods? Much clearer to read loading ? this.renderLoadingIndictator() : this.renderForm() than a huge block of JSX

This just moves the huge chunk of JSX problem elsewhere, I've done it anyway though :)

Haha fair point :) But a huge chunk of JSX isn't a problem in itself, it's just not great hanging off a ternary operator.

WickyNilliams · 2017-06-21T12:39:52Z

src/reducers/index.js

+const createReducer = reducers =>
+  combineReducers({
+    questionnaire,
+    router: routerReducer,


could always put { routerReducer as router } in your import statement if you want to consistently use shorthand properties

samiwel · 2017-06-21T13:50:54Z

src/apollo/client.js

+    return null;
+  },
+  networkInterface: createNetworkInterface({
+    uri: "http://localhost:4000/graphiql"


This should probably be an environment variable.

It bloody should be 🎉

Separate Trello task added to cover it

samiwel · 2017-06-23T12:24:40Z

.env.production

@@ -1 +1,2 @@
 REACT_APP_BASE_NAME="/eq-author"
+REACT_APP_API_URL=""


Are we able to override the REACT_APP_API_URL value with an environment variable. Or do we need to default it to process.env.REACT_APP_API_URL here?

We need to be able to customise this for successful deployment to CloudFoundry.

WickyNilliams

This is so much simpler than we had before! Definitely moving in the right direction 👍

Few little points i think are worth fixing

WickyNilliams · 2017-06-28T08:07:26Z

package.json

@@ -87,11 +88,13 @@
  "dependencies": {
    "connected-react-router": "^4.2.1",
    "enzyme": "^2.8.2",
+    "global": "^4.3.2",


What's this?

An accident!

WickyNilliams · 2017-06-28T08:12:10Z

src/apollo/client.js

+import { ApolloClient, createNetworkInterface } from "react-apollo";
+
+const client = new ApolloClient({
+  ddTypename: true,


s/ddTypename/addTypename

WickyNilliams · 2017-06-28T08:17:05Z

src/components/BaseLayout/index.js

  hasUtilityBtns: false
 };

+BaseLayout.displayName = "BaseLayout";


What's the need for this?

A workaround for this bug which breaks snaphot tests on Travis.

WickyNilliams · 2017-06-28T08:18:01Z

src/components/Breadcrumb/index.js

+  return (
+    <BreadcrumbNav aria-label="breadcrumb">
+      <BreadcrumbList>
+        <BreadcrumbItem key="/">


Shouldn't need key prop, unless elements are in an array.

WickyNilliams · 2017-06-28T08:18:47Z

src/components/Breadcrumb/index.js

-    })
-  ).isRequired
+        <BreadcrumbItem key={breadcrumb.path}>
+          <BreadcrumbLink to={breadcrumb.path}>


Should have the aria-current attribute on the current breadcrumb

Linter complains about using aria-current, apparently it's not in the spec

Hmm that's odd. Appears to be part of the standard here https://www.w3.org/TR/wai-aria-1.1/#aria-current. Perhaps we should raise a bug against the eslint plugin?

WickyNilliams · 2017-06-28T08:36:11Z

src/components/Forms/TextArea/index.js

  id: PropTypes.string,
+  handleChange: PropTypes.func,


onChange should be the name of this kind of prop (here and anywhere else), to align with regular DOM components. handleChange should only be used internally e.g.

class ExampleComponents extends Component { handleChange = (e) { const value = e.target.value; this.props.onChange(value); }; render() { return ( <input type="text" value={this.props.value} onChange={this.handleChange} /> ); } }

WickyNilliams · 2017-06-28T08:39:30Z

src/components/QuestionnaireMeta/index.js

+  return (
+    <Center>
+      <Panel>
+        <Form handleSubmit={handleSubmit}>


as before, this should be onSubmit. onX for external props, handleX for internal method

WickyNilliams · 2017-06-28T08:40:49Z

src/containers/QuestionnaireCreatePage/QuestionnaireCreate.js

+    this.setState(questionnaire);
+  }
+
+  onChange = value => this.setState(value);


as discussed, these should be called handleChange, handleSubmit etc

WickyNilliams · 2017-06-28T08:41:59Z

src/containers/QuestionnaireMetaPage/QuestionnaireMeta.js

+    this.props.onUpdate(this.state);
+  };
+
+  onSubmit = e => e.preventDefault();


WickyNilliams · 2017-06-28T08:43:23Z

src/containers/QuestionnaireMetaPage/index.js

+
+export default compose(connect(mapStateToProps), withData, withMutation)(
+  QuestionnaireMeta
+);


There is a lot of duplicated code between this and src/containers/QuestionnaireDesignPage/index.js. Should extract common queries etc

Think the best way to do so is to implement the webpack loader plugin, can this be done as a separate task?

Added task to trello: https://trello.com/c/pBSrctE2/99-investigate-use-of-webpack-loader-for-graphql-files

WickyNilliams · 2017-06-28T09:14:52Z

Create New Q (8)

WickyNilliams · 2017-06-28T11:54:29Z

src/components/Forms/Form/Form.test.js

@@ -3,11 +3,11 @@ import { shallow } from "enzyme";
 import { Form } from "components/Forms/Form";

 let wrapper;
-const handleSubmit = jest.fn();
+const onSubmit = jest.fn();


These functions should actually be called handleSubmit haha. But don't worry if they're just tests!

WickyNilliams · 2017-06-28T11:56:23Z

src/components/RichTextArea/index.js

@@ -52,7 +52,7 @@ export default class RichTextArea extends Component {
    };
  }

-  handleChange = value => {
+  onChange = value => {


this should be handleChange

hamishtaplin added the In progress label Jun 14, 2017

hamishtaplin force-pushed the 2/create-new-questionnaire branch from 88dc97f to f93b549 Compare June 15, 2017 14:41

bennpearson previously requested changes Jun 20, 2017

View reviewed changes

hamishtaplin removed the In progress label Jun 21, 2017

WickyNilliams suggested changes Jun 21, 2017

View reviewed changes

samiwel suggested changes Jun 21, 2017

View reviewed changes

samiwel reviewed Jun 23, 2017

View reviewed changes

hamishtaplin added 22 commits June 26, 2017 15:25

hook apollio client up to store

fa05b7c

wire questionnaire meta up to api

bf20e6a

handle mutations

16bcc23

mutation onSubmit of form

476a2a0

WIP

10138e2

added debounced updating

f16bce5

Add gql to breadcrumbs

21c4573

remove questionnaire ID

46627b2

minor cleanup

2dcd597

update some tests

e79b677

tests

598f335

checkbox tests

7bd2e0b

more tests

37cec38

enhancer test

d748df1

remove enhancer tests

f132836

appease lint gods

86b2c2d

Update tests

86d5700

correct case on import

41c77d8

revert changes to Breadcrumb component

462e029

address code review comments

a2e6dd0

undo a whoopsie

6af3679

undo another whoopsie

d9e4ad3

hamishtaplin added 9 commits June 26, 2017 15:25

update snapshot

0b956cb

Review changes

15f2b41

slight refactor

d9bec58

more refactoring

68af4e4

simplify routes

9efad9b

proptypes

11f04c9

fix existing tests

7e59d02

tests

78c63d8

More tests

bc59cf7

hamishtaplin force-pushed the 2/create-new-questionnaire branch from 15e01fa to bc59cf7 Compare June 27, 2017 14:11

hamishtaplin added 2 commits June 27, 2017 15:28

update snapshot

3062c9d

disable smoke tests

63dd3b1

WickyNilliams suggested changes Jun 28, 2017

View reviewed changes

hamishtaplin added 2 commits June 28, 2017 12:38

Address PR comments

d183cf5

move queries to external files

64163d8

WickyNilliams reviewed Jun 28, 2017

View reviewed changes

hamishtaplin added 3 commits June 28, 2017 13:02

moar PR comments

e1905af

add Travis config

6fc3591

displayName fix

b1350d5

hamishtaplin merged commit 85da995 into master Jun 28, 2017

hamishtaplin deleted the 2/create-new-questionnaire branch June 28, 2017 13:33

		@@ -1 +1,2 @@
		REACT_APP_BASE_NAME="/eq-author"
		REACT_APP_API_URL=""

Create new Questionnaire (integration) #140

Create new Questionnaire (integration) #140

Conversation

hamishtaplin commented Jun 14, 2017 • edited Loading

Changes

Testing

Choose a reason for hiding this comment

hamishtaplin Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

WickyNilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishtaplin 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

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

hamishtaplin Jun 22, 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

Choose a reason for hiding this comment

hamishtaplin Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WickyNilliams Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

hamishtaplin Jun 22, 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

WickyNilliams Jun 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WickyNilliams left a 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

hamishtaplin Jun 28, 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

hamishtaplin Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

hamishtaplin commented Jun 14, 2017 •

edited

Loading

hamishtaplin Jun 21, 2017 •

edited

Loading

hamishtaplin Jun 21, 2017 •

edited

Loading

hamishtaplin Jun 22, 2017 •

edited

Loading

hamishtaplin Jun 21, 2017 •

edited

Loading

WickyNilliams Jun 22, 2017 •

edited

Loading

hamishtaplin Jun 22, 2017 •

edited

Loading

WickyNilliams Jun 22, 2017 •

edited

Loading

hamishtaplin Jun 28, 2017 •

edited

Loading

hamishtaplin Jun 28, 2017 •

edited

Loading