34 feedback component #61
Conversation
MohammadAlhalaq
added
the
In progress
MohammadAlhalaq
added
Awaiting Review
Relates #34
MohammadAlhalaq
requested review from
alaa-yasin,
ahmedisam99,
ranasobeid95 and
SajedaIsmail
client/src/components/App/index.js
Outdated
| export default () => ( | ||
| <Router> | ||
| <Switch> | ||
| <Route path="/" exact component={Feedback} /> |
There was a problem hiding this comment.
| <Route path="/" exact component={Feedback} /> | |
| <Route exact path="/feedback" component={Feedback} /> |
You should also pass the table number which will be stored in the state after the user logs in as a prop to display it in the feedback page.
Also, find a way to pass the order number to the feedback component (which also will be displayed in it)
| body: JSON.stringify(data), | ||
| }) | ||
| .then(res => res.json()) | ||
| .then(status => { |
There was a problem hiding this comment.
This isn't the status coming from the res.json(), but it is the response's payload
| .then(status => { | |
| .then(payload => { |
or
| .then(status => { | |
| .then(resBody => { |
or
| .then(status => { | |
| .then(response => { |
| .then(res => res.json()) | ||
| .then(status => { | ||
| if (status.statusCode === 201) { | ||
| this.setState({ errorMessage: '' }); |
There was a problem hiding this comment.
why update the state while you are redirecting the user immediately, especially because setState is an async function which means the redaricting will occur before the execution of setState!
| } | ||
| }) | ||
| .catch(err => { | ||
| this.setState({ errorMessage: err }); |
There was a problem hiding this comment.
err's origin here may differ depending on the generator of the error, which here may be from the fetch (network error) or a validation error (generated by @hapi/joi).
According to what I said above in all the cases of an error getting generated the value of err will be an object!!! which is a disaster as you render the whole error object to the user!
| fetch('/api/v1/post-feedback', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data), |
There was a problem hiding this comment.
Use @hapi/joi to validate the user's input before sending it.
| .table-number{ | ||
| width:40vw | ||
| } | ||
| form{ |
There was a problem hiding this comment.
Do not use tag name selectors in css!!
package.json
Outdated
| @@ -36,10 +36,13 @@ | |||
| "dependencies": { | |||
| "@hapi/joi": "^16.1.4", | |||
| "cookie-parser": "^1.4.4", | |||
| "cors": "^2.8.5", | |||
There was a problem hiding this comment.
After using proxy in react there is no need for this module/package
| "cors": "^2.8.5", | |
package.json
Outdated
| "env2": "^2.2.2", | ||
| "express": "^4.17.1", | ||
| "jsonwebtoken": "^8.5.1", | ||
| "pg": "^7.12.1" | ||
| "pg": "^7.12.1", | ||
| "react-router-dom": "^5.1.1", |
There was a problem hiding this comment.
Why is react-router-dom and react-stars installed in the backend?!?!?!?!!?!??!?!?!
It's obviously a mistake and not intended, but you must fix it before pushing it!!
server/app.js
Outdated
| @@ -1,13 +1,14 @@ | |||
| const express = require('express'); | |||
| const cookieParser = require('cookie-parser'); | |||
| const cros = require('cors'); | |||
There was a problem hiding this comment.
no need for cors anymore as I mentioned in a previous comment
ahmedisam99
added
Changes Requested
MohammadAlhalaq
added
Awaiting Review
| .then(res => res.json()) | ||
| .then(payload => { | ||
| return payload.statusCode === 201 | ||
| ? history.push('/') |
There was a problem hiding this comment.
the popup must be shown after the submission success and the button inside this popup will redirect to the home.
ahmedisam99
added
Approved
Create feedback component with style