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

yousra/react/Week1 #5

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

Conversation

YousraElmag
Copy link

@YousraElmag YousraElmag commented Sep 3, 2024

@YousraElmag YousraElmag changed the title yousra/react/Week1 yousra/react/Week1 https://66d778ff6f1557c72fd7e66b--superb-pothos-22b3fc.netlify.app/ Sep 3, 2024
@YousraElmag YousraElmag changed the title yousra/react/Week1 https://66d778ff6f1557c72fd7e66b--superb-pothos-22b3fc.netlify.app/ yousra/react/Week1 Sep 3, 2024
@crevulus crevulus self-assigned this Sep 11, 2024
Copy link

@crevulus crevulus left a comment

Choose a reason for hiding this comment

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

Hi Yousra,

This is a great first assignment. Your app is beautiful and you have a good understand of core React concepts. Great work.

I've left some comments for you to review. Please make the changes I've suggested. In particular, try not to repeat yourself in your code. The more times you repeat yourself, the more likely you are to make a small mistake which will cause data to fall out of sync.

@@ -0,0 +1 @@
/* /index.html 200

Choose a reason for hiding this comment

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

Can you explain what this file is and why you need it?

transition: background-color 0.3s;
}

.category-list button.active,

Choose a reason for hiding this comment

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

Your UI is very nice. Great attention to detail.

filter: drop-shadow(0 0 2em #61dafbaa);
}

@keyframes logo-spin {

Choose a reason for hiding this comment

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

You've left a lot of styles in here that you're not using. Be sure to delete unnecessary CSS. 🟢

onClick={() => onCategorySelect(category)}
className={selectedCategory === category ? "active" : ""}
>
{category.replace("FAKE: ", "")}

Choose a reason for hiding this comment

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

You do this three times: once here, once in productlist.jsx, and once in App.jsx. Can you figure out a way to only do this cleaning function once, and not repeat yourself? 🟠

@@ -0,0 +1,19 @@
// src/components/ProductList.jsx

Choose a reason for hiding this comment

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

The convention in React (and other frontend frameworks) is to name your component files in the same case as your component name - i.e. with a capital letter. Categories, Products, etc. 🟢

Choose a reason for hiding this comment

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

unresolved

<img src={product.image} alt={product.title} />
<h3>{product.title.replace("FAKE: ", "")}</h3>
<p>${product.price}</p>
<p>Rating: {product.rating.rate} (Reviews: {product.rating.count})</p>

Choose a reason for hiding this comment

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

Great attention to detail again.

.gitignore Outdated
# Except the folder you want to keep
!/ecommerce/
!/ecommerce/**/*

Choose a reason for hiding this comment

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

You shouldn't need to edit the gitignore file. Please change this back.

@crevulus crevulus mentioned this pull request Sep 30, 2024
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