-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
London-10_Saqib-Javed_full-stack_level_100_200_300 #379
Conversation
level 100 WIP
Level 100 WIP
completed level 100 and 199
Level 200 - Week 2 - Back End WIP
Level 200 - Week 2 - Back End - completed
client/src/Components/NewVideo.js
Outdated
|
||
const [title, setTitle] = useState(""); | ||
const [url, setUrl] = useState(""); | ||
const [newVideo, SetNewVideo] = useState({ title: "", url: "" }); |
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.
Nice use of destructuring to initialize the newVideo state!
|
||
return ( | ||
<div className="new-video"> | ||
<p>Add new video to your list</p> |
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.
Using an <h3>
element is also a good choice for the title or heading of a section.
return ( | ||
<div className="App"> | ||
<header className="App-header"> | ||
<h1>Video Recommendation</h1> | ||
<div> |
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.
FYI this component does not need to be wrapped in a <div>
but they do need to be wrapped in a single parent element which you did nicely (<div className="App">)
<div> | ||
|
||
<div></div> | ||
<div key={video.id}> |
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.
The key prop should be applied to the outermost element that is part of a list of elements. In your case, it should be applied to the outer <div>
that wraps each video card, not the inner div.
FYI this is important for React to efficiently update the component list.
console.log("this is video", video); | ||
setVideos([video, ...videos]); | ||
console.log("in add videos", videos); | ||
}; |
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.
Good job in using spread operator
! can you also remove unused codes in your PR? console.log
s
server/server.js
Outdated
const videoid = videos.map((video) => video.id); | ||
const id = Math.max(...videoid) + 1; | ||
|
||
videos.push(newVideo); |
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.
when adding a new video, you can also consider what happens if videos.push(newVideo)
fails :)
//Deletes the video with the ID container within the {id} parameter | ||
|
||
app.delete("/videos/:id", (req, res) => { | ||
let id = Number(req.params.id); |
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.
Since id
in your code doesn't need to be reassigned, you can use const
instead.
return ( | ||
<div> | ||
|
||
<div></div> |
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.
Also you don't need this line I guess.
server/server.js
Outdated
return video.id === id; | ||
}); | ||
if (!matchingVideo) { | ||
res.status(400).send("No matching video with this ID exists."); |
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.
Instead of sending a 400 status code for "No matching video with this ID exists." you might consider using a 404 status code which is more suitable here
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.
Great job @saqibjvd!
I've just spotted very minor issues and good luck with DB step and deployment!
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.
This is generally looking good, congrats!
client/src/Components/NewVideo.js
Outdated
|
||
const [title, setTitle] = useState(""); | ||
const [url, setUrl] = useState(""); | ||
const [newVideo, SetNewVideo] = useState({ title: "", url: "" }); |
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.
What is this state for? I don't see it being set or read anywhere?
server/server.js
Outdated
if (!newVideo.title || !newVideo.url || !newVideo.url.startsWith("https://www.youtube.com")) { | ||
res.status(404).send({ | ||
result: "failure", | ||
message: "Video could not be saved", |
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.
It can be useful to include what the specific problem was in this message, so it can be displayed to the user (e.g. that it was specifically the title that was missing, or that the URL wasn't a YouTube URL, etc)
WIP 250 and level 300 one step
level 100 WIP