liron's pr#6
Conversation
|
|
||
| const DropDownFilter: React.FC<DropDownFilterProps> = ({ onClick, isOpen, filterByGenre, genre }) => { | ||
| const categories: Category[] = [ | ||
| export const categories: Category[] = [ |
There was a problem hiding this comment.
-
Exporting categories: You exported categories, which is good for reusability. However, keep in mind:
1.1. If categories is only used in this component, exporting might introduce dead code elsewhere.
1.2. If it is reused (as we see in Card), this solution is appropriate. Consider adding a comment or utility
folder structure for constants (like categories). -
Performance Optimization: The categories array is recreated on every render, which is unnecessary. Instead:
You can define it as a const outside the component to ensure it's only created once.
export const categories: Category[] = [...]; // Move outside the function.
| <p><strong>Category: </strong></p> | ||
| <p><strong>{`Price: ${props.product.price}`}</strong></p> | ||
| <p><strong>{`Rating: ${props.product.rating.rate}/${maxRatings} (${props.product.rating.count} Reviews)`}</strong></p> | ||
| <p><strong>{`Category: ${categories.find((category) => category.id == props.product.category)?.name}`}</strong></p> |
There was a problem hiding this comment.
Accessing categories:
Good use of categories.find. However, watch out for cases where find might return undefined. To make it more robust, provide a fallback value:
const categoryName = categories.find(category => category.id == props.product.category)?.name || 'Unknown Category';
|
|
||
| const Card: React.FC<CardProps> = () => { | ||
| const Card: React.FC<CardProps> = (props: CardProps) => { | ||
| const maxRatings = 5 |
There was a problem hiding this comment.
Magic Numbers (maxRatings):
Use constants or make the logic more descriptive.
const MAX_RATINGS = 5;
| <div className='product scale-effect'> | ||
| <div className='product-image'> | ||
| <img /> | ||
| <img src={props.product.image}/> |
There was a problem hiding this comment.
Error Handling for Loading Image:
props.product.image is used directly. If the image URL fails, it might display a broken image. Add an onError:
<img src={props.product.image} alt={props.product.title} onError={(e) => (e.target.src = '/placeholder.png')} />
| } | ||
|
|
||
| const Card: React.FC<CardProps> = () => { | ||
| const Card: React.FC<CardProps> = (props: CardProps) => { |
There was a problem hiding this comment.
Destructure Props:
Destructure props to improve readability:
const { product } = props;
nadav-cohen11
left a comment
There was a problem hiding this comment.
Great job on adding more functionality:
Loading data from the fakestoreapi with axios.
Creating reusable categories.
Making the Card component dynamic by adding props.
Attention to detail is important:
Properly componentized and streamlined logic for filtering products.
Nice work cleaning up TODO comments.
Please fix the comments that i wrote you and open the pull request again
couldn't find Details.tsx