Skip to content

Conversation

@SyedShahriar
Copy link

This MR is for reviewing the project code

// for sidebar home
const homePage = () => {
setSearchQuery('');
setViewMode('all');
Copy link
Author

Choose a reason for hiding this comment

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

Lets add a comment as to why we set the view Mode here to 'all'

Choose a reason for hiding this comment

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

Added a more explanatory comment on why

};

// for sidebar home
const homePage = () => {
Copy link
Author

Choose a reason for hiding this comment

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

Should we rename this, because its for the side bar on the homepage. This way it better represents what we are setting up for.

Copy link

@Jenn-jaee Jenn-jaee Jun 16, 2025

Choose a reason for hiding this comment

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

Changed the comment to reflect where the homepage is located for better clarity

}}
>
<img src= {hasWatched ? '../assets/eye-icon.jpg' : '../assets/close-eye-icon.jpg'} alt=" Watch Status" />
{/* <img src = {eyeIcon}/> */}
Copy link
Author

Choose a reason for hiding this comment

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

Why are lines 36-38 commented out?

Copy link

@Jenn-jaee Jenn-jaee Jun 16, 2025

Choose a reason for hiding this comment

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

I couldn't find the commented reference you mentioned here in my coding environment, I'm thinking the changes were already cleaned up.
However, the commented lines was updated to a more workable way of accessing my icon for watch and not watch display as the former wasn't displaying the icon on my webpage

@SyedShahriar
Copy link
Author

In general great job! The code looks good overall. Some general tips:

  1. Lets make more incremental commits instead of combining a bunch of commits together e.g. the first commit here fixed nav bar issue, allow for overlay display and my home page rende… was addressing multiple things in one commit, so there was an opportunity to split up the commits here.
  2. Lets try to assign more meaningful variable names

@Jenn-jaee
Copy link

Thank you so much for the Feedback!
Would keep these in mind when working on future projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants