This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://websiteone-fe-git-fork-jmpainter-189-share-hangout-links.agileventures1.now.sh

great @jmpainter thanks for getting started on this so quickly and making some real progress!

Do we need to turn EventSummary into a class component?

I understand that you did it to calls setState, but I guess I was thinking we could actually have the form be itself a functional component.

Are you concerned about calling handleChange from the child or grandchild?
I think so far in the project we have tried to keep components as functional as far as possible.

Just some ideas, and open for discussion and what others think, but I wanted to get back to you.
I really like that you listed the changes you made in each file. Also, I like that you have thought about specific use cases like the user being logged in, trying to share a blank link 😸

I did find this article, which I haven't completely gone through, but I did see disables the button unless there is something added to the form input.

• EventInfo - state related to Hangout link form added
• EventSummary - returned to functional component
• SingleFormField - component added, could be used in other places where a form like this is desired

Yes, this is a much better setup

hey @jmpainter, this is currently failing the build with this output

#!/bin/bash -eo pipefail
yarn test
yarn run v1.9.4
$ export NODE_ENV=testing && standard && jest
standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /root/WebsiteOne-FE/src/components/EventSummary.js:1:27: 'Component' is defined but never used.
  /root/WebsiteOne-FE/src/components/EventSummary.js:2:46: 'Form' is defined but never used.
  /root/WebsiteOne-FE/src/components/EventSummary.js:2:52: 'Button' is defined but never used.
  /root/WebsiteOne-FE/src/components/EventSummary.js:11:31: Block must not be padded by blank lines.
  /root/WebsiteOne-FE/src/components/SingleFieldForm.js:13:12: Extra semicolon.
  /root/WebsiteOne-FE/src/components/SingleFieldForm.js:36:31: Newline required at end of file but not found.
  /root/WebsiteOne-FE/src/containers/EventInfo.js:16:20: Missing space before function parentheses.
  /root/WebsiteOne-FE/src/containers/EventInfo.js:26:28: Missing space before function parentheses.
  /root/WebsiteOne-FE/src/containers/EventInfo.js:45:9: Missing space before function parentheses.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Exited with code 1

I think you know this, but you can run yarn fix before pushing up... Also, it's good to get in the habit of running the tests locally with yarn test, which run standard first and fail if there are any linting issues.

SingleFieldForm.test.js - added with tests for SingleFieldForm
EventInfoTest.js - added test for postEventLink
EventSummary.test.js - added test for SingleFieldForm render

EventSummary.js - returned to un-Redux connected component (no longer needed since EventInfo is dispatching the postEventLink action)

Not sure about Coveralls

hey @jmpainter, I am gonna see if I can get this working with the backend, as we discussed.

So there were some merge conflicts that I did a rebase locally, but when you rebase you need to force push, so you might notice I was added as a co-collaborator on your other commits, hope that is ok.

Yes, sure - no problem.

@jmpainter, just to keep you in the loop with the progress I've made.
It's now working to post events to a our live slack instance!!

If you test it out, please only create events associated with the project Project for testing slack notifications, otherwise it will spam one or more of our live channels.

I removed your validation for an empty field in favor of using semantic-ui-react required prop, which does a similar thing.

Also, made some other changes, let me know if you have any doubts as to why, or if you think I can revert any changes I made.

At the moment, the only thing(potentially) keeping this from getting merged in is testing the postEventLinkAction, I need to think about it cause at the moment, I am not dispatching anything to the store, since we don't need to use it for anything. I should dispatch any error message coming from the backend, maybe just testing that will be enough for our coverage(?)

Thanks for getting this working with the back end. Looks good! I'll pull it down and take a look at it tomorrow to take a look at it and will see about writing the test for postEventLinkAction.

It looks like right now we are not looking for any response coming from the /hangouts/:id endpoint in the postEventLinkAction. Would it be good to look for a success response and display a message to the user?

Perhaps, at the moment in production we don't do that, but I wouldn't be opposed to it.

I know it was successful cause Slack notifies me. We should definitely show an error message if there is one.

I see - maybe the success message isn't necessary because this is more of an internal use thing. An error message could be useful. Right now SingleFieldForm's 'error' prop is not connected to anything. Maybe we should be using redux-form for SingleFieldForm since it is used elsewhere in the project. Or the error prop could be passed down from EventInfo.js - or maybe none if this is worth it right now?

I think error handling would be nice, we could maybe just show a toaster with the error message, what do you think?

If redux-form is intended to be used on all forms in the project, I'd go with that on singleFieldForm for consistency. Otherwise I'd go with passing the error prop to SingleFieldForm from EventInfo because this would be the simplest, is consistent with the redux-form approach, and would put the error message by the field. If other forms in the app are putting error messages by the responsible fields, I think that would be what the user would expect.

I think it depends on the type of error, no?
If the error is to do with frontend validations, then I would agree.
However, if the error comes from the backend, then we certainly have several examples of using toasters throughout the code base.

Have a look at the Login container, for example

I see it now - had missed it before in a search. Then I'd go with the toast because that is the established pattern for the user and the boilerplate is in place. Redux-form generally doesn't work that way - the errors come from the backend into the store and come into the form to be displayed by the fields. So far only ProjectForm is using redux-form. I suppose that it could produce toasts instead. Probably it would be best to create all forms the same way - not sure what the long range plan is.

I would agree it would be nice to have the forms use the same.
What I have seen mostly the form errors used for is validations and they are displayed onChange, but then if the backend responds with a Unauthorized, or 500 Internal Server Error that they would show up in a toaster.

But I'm not sure how the Redux-Form errors work, like if the backend responds saying some field is required, it shows up in the field's error? For me, it would be better to have frontend validations that don't allow a user to submit the form if there are certain validations that the backend enforces, you know?

Redux-form is set up to use custom front end validators as well and feeds the errors from the front end and back end back to the form where they are displayed with the appropriate field. You can also have an overall form error displayed from the back end like for a 500 error.

@joaopapereira, @jmpainter and I discussed this PR in the meeting after you dropped off, and we were thinking it might be best to hold off on merging it until we can write a test for the postEventLinkAction

Now that we are using Redux-Form, we were talking about all new forms be created with it, so we'd need to refactor the SingleFieldForm to use it.

We had a doubt about error handling, should we be using Redux-Form's error handling, or should we be using toasters as we are in other places. I imagine that we could have refactoring tickets, which wouldn't have high priority, to use Redux-Form for all our forms, which means we could refactor out the toasters if we decide to use the native error handling form Redux-Form.

What do you think?

I am ok with holding off the merge

Any progress on this @jmpainter??