Ilias_Khugaev-w1-React #3
Conversation
| import allItems from "./fake-data/all-products.js"; | ||
| import { useState } from "react"; | ||
|
|
||
| function CategoryBar({ items, selectedCategory, onSelectCategory }) { |
There was a problem hiding this comment.
You are missing a requirement from the README:
- There should only be 1 category active at a time and the user should see which category is selected.
🔴
There was a problem hiding this comment.
i really need help with implementing a feature where only one category can be active at a time, and the user should be able to see which category is currently selected. Right now, I'm thinking of just adding a class="selected-category" to the clicked button, but I'm wondering if there's a cleaner or more efficient way to do this. Could you suggest a better approach?
| } | ||
|
|
||
| li:hover { | ||
| background-color: hsl(0, 0%, 85%); |
There was a problem hiding this comment.
Using different colours for hover states is great attention to detail, but the change here is probably too subtle. You might want to make the contract between normal state and hover state a bit more obvious for better UX. 🔵
|
|
||
| } | ||
|
|
||
| h1 { |
There was a problem hiding this comment.
Be careful about applying styles to all elements or all elements of one type across your application. There may come a time when you want to use different styles, and that will be very difficult because of the styles you've set here.
There's a good example of this in your app already. You've applied hover styles to all list items because your buttons are list items. However, your product cards are also list items. So they have hover effects but they're not clickable, leading to bad UX. 🟠
| @@ -0,0 +1,84 @@ | |||
| html { | |||
| font-size: 62.5%; | |||
There was a problem hiding this comment.
I believe that we should stick to 16px. Here's a good series of comments as to why that's a good idea (in my opinion): https://www.reddit.com/r/webdev/comments/fhfe0n/when_using_rem_is_it_better_to_have_a_16px_base/
I will let you do what you think is best with the font size. As for the other css properties (not just on * but also on h1, li, span) you should consider the maintainability of your project going forwards - this is what employers will consider as well.
There was a problem hiding this comment.
Thank you very much for your comment, I really appreciate your perspective. For me personally, the exact font size doesn’t play a huge role for me. Honestly, I just saw that using rem is considered modern practice, and as a beginner, I'm still figuring out the best approach to choosing font sizes.
In the future, I will definitely refer more to what is expected by the employer or the team I work with. For this project, though, I’ll leave it with rem as a bit of an experiment and learning opportunity.
| @@ -0,0 +1,20 @@ | |||
| function AllProducts({ products, filter = null}) { | |||
There was a problem hiding this comment.
Is this component name accurate? When I apply a filter, I'm no longer seeing all products. But the component is called all products.
Also make sure your file names are consistent. use kebab-case or PascalCase for all of them. 🔵
| return ( | ||
| <div className="category-bar"> | ||
| <ul className="category-list"> | ||
| {items.map((bar, index) => ( |
There was a problem hiding this comment.
Why is the first parameter named bar? Would you say that each item in the list of categories is one "bar"? 🔵
week1/project/ecommerce/src/App.jsx
Outdated
| function App() { | ||
|
|
||
| return ( | ||
| <Navbar></Navbar> |
There was a problem hiding this comment.
Components with no children can be self-closing: <Navbar/> 🔵
week1/project/ecommerce/src/App.jsx
Outdated
| @@ -0,0 +1,10 @@ | |||
| import Navbar from './Navbar.jsx'; | |||
| import AllProducts from './all-products.jsx'; | |||
| <> | ||
| <h1>Products</h1> | ||
| <CategoryBar items={categories} onSelectCategory={setSelectedCategory} selectedCategory={selectedCategory} /> | ||
| <AllProducts products={allItems} filter={selectedCategory} /> |
There was a problem hiding this comment.
You're rendering AllProducts inside the Navbar. But this component isn't at the top of the page and isn't a part of navigation. Try rendering it somewhere more logical. Think about the page layout and whehter components should be inside components or next to components. 🟠
https://ilias-khugaev-week1-react.netlify.app/