-
Notifications
You must be signed in to change notification settings - Fork 7
LidiiaStarshynova-w1-React #1
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
base: main
Are you sure you want to change the base?
LidiiaStarshynova-w1-React #1
Conversation
Hi Lidiia, Your code works well and shows a good understanding of React. Great work! Assignment Requirements ✅
Comments Rules
Suggestions 💡
Nice-to-have ✨
--> General: Looking at App.js should give you a clear overview of your page structure and component hierarchy. It's like reading the table of contents for your app. Praise 🌟
Nice one! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
It should be Uppercase, as with buttonSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @starshynova here is the example to comment
fixed in 300bd10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Removing unused files that came with the project template. such as index.css
, and assets folder
week1/project/ecommerce/src/Card.jsx
Outdated
@@ -0,0 +1,10 @@ | |||
const Product = (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Product = (props) => { | |
const Card = (props) => { |
Suggestion:
Better to make the component name match the file name
@@ -0,0 +1,15 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import React from 'react'; |
Suggestion:
When importing a React component, you can skip 'import React' for modern React (17+)
Reference
week1/project/ecommerce/src/App.jsx
Outdated
const [filterProducts, setFilterProducts] = useState(allProducts); | ||
|
||
const handleFilterProducts = (category) => { | ||
const filtered = allProducts.filter(product => product.category === category.replace(/^FAKE:\s*/, '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
replace(/^FAKE:\s*/, ''))
ideally be handled within ButtonSet
or Button
.
Due to the Single Responsibility principle, The App pages shouldn't need to know the 'FAKE:' detail.
week1/project/ecommerce/src/App.jsx
Outdated
return ( | ||
<main> | ||
<h1>Products</h1> | ||
<ButtonSet setFilterProducts={handleFilterProducts} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ButtonSet setFilterProducts={handleFilterProducts} /> | |
<ButtonSet onCategorySelect={handleCategorySelect} /> |
Suggestion:
For general naming in React, props that handle events should start with 'on'.
- The 'set' prefix is typically reserved for useState hook naming
- The name should describe what the prop does, not how it does it
Reference
No description provided.