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

Assignment-week3-React-Alia Taher #33

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Alia-Taher
Copy link

No description provided.

@MaryRichelle MaryRichelle self-assigned this Nov 28, 2024
Copy link

@MaryRichelle MaryRichelle left a comment

Choose a reason for hiding this comment

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

Nice work overall!
I noticed that the useFetch hook is in the main folder. It would be better to create a useFetch.js file and move it to a hooks folder. Also you can move context files into cotext folder. This will help keep the project organized and make it easier to manage.

<div className="Fav-page">
<h2>Favorites</h2>
<NavLink to="/favorites" name={"Favorites"} className={"fav-link"} />
<NavLink to="/" name={"Products"} className={"products-link"} />

Choose a reason for hiding this comment

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

Nice work! I noticed you're repeating the same code for the navbar on all pages. It would be great if you could refactor this and make the navbar appear consistently across all pages by creating a single navbar component in React.

Copy link

@MaryRichelle MaryRichelle left a comment

Choose a reason for hiding this comment

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

👏👏

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.

2 participants