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

Glasgow 6 | Haroun Alarabi | Full-Stack-Project-Assessment | Level 300 #393

Closed
wants to merge 155 commits into from

Conversation

HarounAlarabi
Copy link

@HarounAlarabi HarounAlarabi commented Sep 3, 2023

https://verdant-crisp-d6d210.netlify.app/
Hi @Dedekind561 I'm attempting to display data from Render in browser but i's not showing up. However, when I use Postman to retrieve the data from Render, it works as expected."

@Dedekind561 Dedekind561 added the in progress Mentor review in progress label Sep 12, 2023
@Dedekind561
Copy link
Contributor

Hi @HarounAlarabi ,
Thankyou for submitting Level 300 of the Full Stack Project Assessment. However, at the moment, I can't review your work properly. Locally I'm just seeing a blank screen with a heading Video Recommendation - do you have a deployed version of the site itself?

@Dedekind561
Copy link
Contributor

Morning Haroun,
Thankyou for submitting a link to your deployed application.
At the moment, I can't review your work because you've committed the node_modules. Please can you remove them from your pull request then I can take a look :)

@Dedekind561
Copy link
Contributor

The deployed version of your app is now working for me at the moment :)

@HarounAlarabi
Copy link
Author

Hi @Dedekind561 I have commited the code again with changes

Copy link
Contributor

@Dedekind561 Dedekind561 left a comment

Choose a reason for hiding this comment

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

Hi Haroun,
Thanks for your submission,
I've just taken a look at your code.
The deployed application has the core functionality; however, you need to look more carefully at your logic when adding a new video. Think carefully how the state of your application updates after you've made the POST request. Write down the steps on your piece of paper and think about it carefully to see if you can identify the issues.

Well done on your work so far though 🎉

rating: 0,
};

onVidAdd(newInput);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear to me what this function does just by reading its name onVidAdd

const [searchResult, setSearchResult] = useState([]);

useEffect(() => {
fetch("https://node-server-full-stack.onrender.com/videos")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an environment variable for this API url - don't just hardcode the string. Do you understand why this is important?

Copy link
Author

Choose a reason for hiding this comment

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

I chaned this

@@ -0,0 +1,8 @@
const { Pool } = require("pg");
const videosPool = new Pool({
connectionString: "postgres://full_stack_database_jp62_user:JfOA8mzl7maxtJEnTL28nmslJ5JSqVfe@dpg-ck2p2hvqj8ts73fm2m5g-a.oregon-postgres.render.com/full_stack_database_jp62",
Copy link
Contributor

Choose a reason for hiding this comment

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

❗ You've just made your database connection string public! You need to store strings like this in env variables 👀

Copy link
Author

Choose a reason for hiding this comment

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

I have tried before to use variables to store it but I got an error then I used this way


form{
margin: 20px;
max-width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This CSS rule isn't indented properly: how can you fix this 🔨 ?

.then((res) => res.json())
.then((data) => {
setVideos(data);
searchResult(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you're fetching the data and then you

  1. call res.json()
  2. call setVideos with the data
  3. and then call searchResult -> can you explain the issue here?

Copy link
Author

Choose a reason for hiding this comment

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

the issue here is I used searchResult , so I have to change in to setSearchResult()

body: JSON.stringify(newInput),
})
.then((res) => {
fetchVideos();
Copy link
Contributor

Choose a reason for hiding this comment

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

Call to fetchVideos here - describe the effect of calling this function here

Copy link
Author

Choose a reason for hiding this comment

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

fetchVideos to update the list of videos after adding new video

return res.json();
})
.then((data) => {
if (data && data.message === "Video stored successfully") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you're calling setVideos down here as well - I think you need to think very carefully about what happens when after you've POSTed a new video to your database

HarounAlarabi and others added 29 commits February 13, 2024 15:40
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
Signed-off-by: Haroun Alarabi <[email protected]>
@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
in progress Mentor review in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants