London10 | Seyyednavid Hejazijouybari | React | Hotel-Project(Week1 - 3)#583
London10 | Seyyednavid Hejazijouybari | React | Hotel-Project(Week1 - 3)#583seyyednavid wants to merge 11 commits into
Conversation
seyyednavid
changed the title
|
Kudos, SonarCloud Quality Gate passed!
|
BGIvanG
left a comment
BGIvanG
left a comment
There was a problem hiding this comment.
Overall the website functions great and looks really good I liked the reset button. The code looks good aswell. Just be careful with the gitignore file since removing that caused the push of the node_modules which makes it hard to review the code in the normal way and required me to use the commits.
There was a problem hiding this comment.
Removing the Gitignore (which is used to tell github what files not to include when commiting or pushing)from the project causes the node_modules to be committed which makes it hard to review and also cause the upload time to increase. This is a nice blog explaining it a bit more https://dev.to/grawl/why-you-dont-have-to-commit-nodemodules-folder-33nm.
| @@ -1,15 +1,21 @@ | |||
| import React from "react"; | |||
|
|
|||
| import Header from "./Heading" | |||
There was a problem hiding this comment.
Its normally best practise to name the component and the file the same thing since it makes searching for them much easier.
|
|
||
| const Heading = (props) => { | ||
| return ( | ||
| <React.Fragment> |
There was a problem hiding this comment.
Normally, its easier to just be able to use <> instead of React.Fragment in these situations.
| import londonImg from "./images/london.jpeg"; | ||
| import glassgowImg from "./images/glasgow.jpeg"; | ||
|
|
||
| const TouristInfoCards = (props) => { |
There was a problem hiding this comment.
If no props are being used inside of the component it doesnt need to have the props keyword here.
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetch("https://cyf-react.glitch.me") |
There was a problem hiding this comment.
It would be good to add a .catch to this api request to handle errors.
| const Order = (props) => { | ||
| const [orders, setOrders] = useState(0); | ||
| const orderOne = () => { | ||
| setOrders((prevOrder) => prevOrder + 1); |
There was a problem hiding this comment.
This is currently working great however if you wish to shorten it you can just do prevOrder + 1 without requiring the arrow function
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetch(`https://cyf-react.glitch.me/customers/${userID}`) |
There was a problem hiding this comment.
Would be great to add error handling on this file.
| </thead> | ||
| <tbody> | ||
| {props.results.map((person) => { | ||
| const startDate = moment(person.checkInDate); |
There was a problem hiding this comment.
For this it might be best to have the calculation above the jsx so that the logic and jsx is seprate
3 participants
No description provided.