-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
implemented front-end for contacts page #477
Conversation
Alright so I got a ton of linting errors for my jsx files. It seems like I don't have the linting software for front-end installed... |
hey @rgao, you just need to run |
fixed the linting errors |
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.
Looks good, just a few small changes.
… removed express from package.json, deleted backend changes
Alright, did all of the above @adamkendis Minor annoyance: my input validation checks for only if the fields have input, For email, HTML5 natively checks the email format, but I'm not sure how to integrate that into a conditional in js |
Looks good @rgao . We can beef up the validation or use some validation library before launch if necessary. Okay to update this branch with latest dev so I can merge? |
Alright, updated |
Going to hold off on merging for now. I pushed the github code in PR #481 . Could you finish up the form to send the data to /feedback? The request body should look like:
} We're also using redux-saga to make all of our calls at the moment. We don't necessarily need to store the form data in redux, but it would be ideal to make the axios call to |
@adamkendis @sellnat77 |
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.
Hey Ryan, left a few comments. Thanks!
hey @rgao, one other thing. Sorry to ask for more work, but it's pretty easy. Right now all of the styles in All you need to do to avoid that is nest all of the styles in that file under the |
Alright, working on all these changes |
i defer to @jmensch1 and @adamkendis haha i looked it over though, once they approve we can merge this in |
The comparison tool made a lot of changes so once you resolve your merge conflicts we can push this to dev and catch any issues during QA. |
there are two linting errors but that's due to my linting dependencies being broken
Seems like there's a dependency conflict |
Alright that's my final push for tonight. @adamkendis The linting issues should be solved; I think it's better if you take over any potential problems with redux here |
@brodly @jmensch1 @sellnat77 @rgao
|
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.
that's some bad-ass error handling
#447
I pushed my backend file for dealing with the github API as well, though it's useless for now. I'm also looking into integrating Sanic's email sending package.
@adamkendis @jmensch1
![Screenshot from 2020-03-29 02-01-15](https://user-images.githubusercontent.com/6467379/77845154-56306500-7161-11ea-93f8-355674f84592.png)
Here's what most of the page looks like on my localhost