-
Notifications
You must be signed in to change notification settings - Fork 11
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
Oleksandr week 1 react assignment #9
base: main
Are you sure you want to change the base?
Oleksandr week 1 react assignment #9
Conversation
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.
A good start! Some general comments:
- I like your component splitting, seems very logical!
- If React doesn't need to 'react' to something then it should not be in the state. Remember that every change in state causes a rerender so we want to minimize it if possible
- In the example I can
deselect
a category by clicking on it a second time to get the full list again, but I can't in yours. Can you look into this and see if you can add that feature?
Good luck!
|
||
if (selectedCategory) { | ||
filteredProducts = products.filter( | ||
(product) => product.category === selectedCategory.slice(6) |
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.
Cool solution!
import { useState } from "react"; | ||
|
||
const CategoryList = ({ categories, setCategory }) => { | ||
const [selectedCategory, setSelectedCategory] = useState(null); |
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.
You also have this state in your App
component. Why are there two?
week1/project/ecommerce/src/App.jsx
Outdated
|
||
function App() { | ||
const [categories, setCategories] = useState(allCategories); | ||
const [products, setProducts] = useState(allProducts); |
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.
You never set these state variables so I don't think you need to have them? If it never changes it doesn't need to be in the React ecosystem
Hello Rob, thank you for the feedback. I removed unnecessary states and added the |
Nice work! Good luck with week 2 |
https://thunderous-sfogliatella-b47fd5.netlify.app/