Ahmad_Zetouny-w1-React #5
Conversation
| } | ||
|
|
||
| .categories-tabs li:hover { | ||
| background-color: #6279b8; |
There was a problem hiding this comment.
Good attention to detail to have hover and active colouring, but you might want to make them separate colours so that the user can easily distinguish between the two states. 🔵
| .categories-tabs li { | ||
| width: 100%; | ||
| flex-grow: 1; | ||
| padding: 0.7em 2em; |
There was a problem hiding this comment.
You switch between several unit strategies here. You use both em and rem for dynamic units. I suggest you pick one and stick with it for better maintainability. 🔵
| const categoriesList = categories.map((category) => { | ||
| const filterName = category.slice(6); | ||
| return ( | ||
| <li |
There was a problem hiding this comment.
These should be buttons, not list items. You can put the buttons inside list items, but the element the user clicks should be a button. https://benmyers.dev/blog/clickable-divs/ 🟠
| )); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
You don't need this empty fragment <> - it's not serving any purpose 🔵
| border-radius: 5px; | ||
| } | ||
|
|
||
| .product img { |
There was a problem hiding this comment.
Instead of targeting the html elements within a class, you should simply set a class on the element you want to target. This will do a couple of things: 1. It will make it clearer which targets you're applying styles to, 2. It will increase the specificity of your styles. 🟠
There was a problem hiding this comment.
Thanks for the advice. But in this case, should I keep the classes nested, or should it be seperated from the parent?
.product .product-image{}
OR
.product {}
.product-image{}There was a problem hiding this comment.
up to you! I usually separate them. It depends what you want to do with your styles. Do you want to a) target all product-image classes inside product classes, or target all product-image classes in the whole app? At this early stage they probably have the same result, but try thinking about the long term.
| const filterName = category.slice(6); | ||
| return ( | ||
| <li | ||
| className={filter === filterName ? "active-tab" : ""} |
There was a problem hiding this comment.
There is an error in your console here! 🔴
|
Thank you Chris for checking out my code. I've updated the app: Please let me know if everything is correct so I can move to the 2nd week's assignment. Thanks again. |
|
@Zetouny just so you know, you don't have to wait to correct my feedback before you move on to week 2. You can correct them ask you go forwards with the weekly assignments. |
https://react-week1-ahmad-zetouny.netlify.app/