Skip to content
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

London10_Jan_Softa_Full-Stack_Project_Assessment_Level300 #362

Closed
wants to merge 35 commits into from

Conversation

softacoder
Copy link

No description provided.

@softacoder softacoder changed the title London10_Jan_Softa_Full-Stack_Project_Assessment London10_Jan_Softa_Full-Stack_Project_Assessment_Level300 Aug 24, 2023
import "./App.css";
import VideoComponent from "./VideoComponent";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a React application, it's generally not a good practice to add lots of functions directly in the App.js file which are not going to be re-used across other components.
The App.js file should primarily focus on rendering the main components and managing the state of your application.
I'm referring to handlers functions

@@ -37,6 +37,44 @@ const VideoComponent = ({ video, onUpVote, onDownVote, onRemove }) => {

export default VideoComponent;

// version5 this works on all. but not same size youtube videos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good not to commit commented out codes to remote

@@ -0,0 +1,26 @@
// Get the links and add event listeners
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, you've used vanilla JavaScript to manage links on the page. While it certainly works, in a React application, we typically handle events and interactions using React's built-in capabilities. Is there a specific reason for choosing this approach over using React's event handling?

<div className="video-component">
<h3>{video.title}</h3>
{/* Replace the iframe tag with ReactPlayer */}
<ReactPlayer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to use ReactPlayer component here, I've just noticed, it hasn't been installed. => https://www.npmjs.com/package/react-player
You have installed axios for instance, not this one though


app.listen(port, () => console.log(`Listening on port ${port}`));

// update6
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoide pushing comments which are for yourself, and not info for other devs.

server/server.js Outdated

app.post("/videos", (req, res) => {
const newVideo = req.body;
newVideo.id = videos.length + 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it increasing lenght of video's array?


// // GET "/"
// app.get("/", (req, res) => {
// // Delete this line after you've confirmed your server is running
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of commented codes! 😄

server/server.js Outdated
videos = videos.filter((v) => v.id !== id);
if (videos.length < initialLength) {
res.json({});
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice handling of response! 👏

@@ -1,4 +1,297 @@
<!DOCTYPE html>
<html lang="en">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically in a React application, it's not recommended to add code directly to the index.html file. This source could be helpful to find out the reason for that.

Copy link

@migmow migmow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jan - left some comments. Let me know if there's question. 🙏

@migmow migmow added the Reviewed label Sep 1, 2023
I did some small stuff - added hamburger menu, FAQ, ToA. And removed and updated the previous code.
Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have here is generally looking good, but the PR title says it's level 300 and I only see 299 in here? Do you have 300 somewhere that's missing?

timestamp: new Date().toISOString(),
};

onAddVideo(newVideo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on is a slightly strange prefix for this function name - typically we'd use an on prefix to show something is an event handler ("happens when a video is being added"), rather than the way we actually do the thing (add a video)...

(The same applies to several other names in this PR :))

title={video.title}
width="560"
height="315"
src={`https://www.youtube.com/embed/${video.url.split("v=")[1]}`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if the video URL didn't include v=? Is there anything we could do to make that better?

const [isMenuClicked, setIsMenuClicked] = useState(false);

const updateMenu = () => {
if (!isMenuClicked) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these states all only ever get changed at the same time - could you think of a way to only have one state rather than three?

@softacoder
Copy link
Author

softacoder commented Sep 19, 2023 via email

@softacoder
Copy link
Author

softacoder commented Sep 19, 2023 via email

@illicitonion
Copy link
Member

What you have here is generally looking good, but the PR title says it's level 300 and I only see 299 in here? Do you have 300 somewhere that's missing?"

See screenshot for instructions on level 300. It is deployed on render.

Level 300 requires having integrated with a database, and I don't see any SQL in this pull request - is it possible you have some code locally which you haven't pushed to the PR?

@softacoder
Copy link
Author

softacoder commented Sep 20, 2023 via email

@softacoder
Copy link
Author

softacoder commented Sep 20, 2023 via email

@illicitonion
Copy link
Member

If you have a look at the "Success criteria" on https://github.com/CodeYourFuture/Full-Stack-Project-Assessment/blob/main/300.md it says that "When this stage has been completed all of your data should exist only in your database - no data should be stored in your API or in your React app"

Right now, your code only stores data in the API server - in server.js you have a videos array, and that's where all of your videos are stored. If you deploy a new version of your server, or if render decides to restart your server, all of the data people have uploaded will be lost.

I understand you said you've created a database - that's a good step, but as far as I can tell your server isn't using the database.

If you have a look in server.js in https://github.com/CodeYourFuture/Full-Stack-Project-Assessment/pull/435/files you'll see that that project has changed its handlers so that it looks up information in the database, and when things are added or deleted they're added/deleted in the database rather than in an array.

After you set up your database, did any data get put into it? What's your database being used for?

@softacoder
Copy link
Author

softacoder commented Sep 20, 2023 via email

@softacoder
Copy link
Author

softacoder commented Sep 23, 2023 via email

@softacoder
Copy link
Author

softacoder commented Sep 23, 2023 via email

Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks for taking the time to wire the backend up to the database!

const values = [id];

await db.query(query, values);
res.json({ message: "Downvoted successfully" });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this kind of endpoint where the client hasn't supplied the value to set, we often would return the new value in the response, so that the UI can display the new value (which would factor in that other people may have changed the rating too), and means we can avoid replicating the logic on the client side to do the 0-clamping.

@softacoder
Copy link
Author

softacoder commented Sep 26, 2023 via email

@Dedekind561 Dedekind561 closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants