-
Notifications
You must be signed in to change notification settings - Fork 10
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
Christina react w1 #13
base: main
Are you sure you want to change the base?
Christina react w1 #13
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.
Hi Christina,
Good job on the assignment! I like that you use map
and filter
methods and the styling of the website is nice overall.
I've added a few comments where I thought the code can be made cleaner or more intuitive. The biggest thing for me is making sure the category selectors behave as required in the assignment instructions - namely, that it's possible to also un-select an already selected category by clicking the button for a second time and highlighting the category that's currently selected.
If you get stuck and would like some help, feel free to reach out on Slack!
Cheers,
Michal
PS. Also would be good if you deploy your app somewhere online, such as Netlify (or I personally quite like Vercel)
week1/project/ecommerce/src/App.js
Outdated
import { useState } from "react"; | ||
import ProductList from "./components/Products/ProductList"; | ||
import CategoryList from "../src/components/CategoryList"; | ||
import "../src/App.css"; |
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 don't need to include the '../src' bit in the path - this is telling the app to go one level higher in the folder structure and then return to the 'src' folder. The path used with ProductList is enough.
import React from "react"; | ||
import categoriesData from "../all-categories"; | ||
|
||
const CategoryList = (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.
This works but I recommend looking up props destructuring in React which allows for a more streamlined code.
In this case, it would look something like the following:
const CategoryList = ({ onFilterProducts }) => {
[...]
{<li key={index} onClick={() => onFilterProducts(category)}>
[...]
}
The same applies to the Product & ProductList components.
week1/project/ecommerce/src/App.js
Outdated
const [category, setCategory] = useState(""); | ||
|
||
const filterProducts = transformProducts.filter((product) => { | ||
return product.category === category || category === ""; |
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.
Instead of using the map method to update the original product data, I would prefer if you modify the string - similarly to what you now do in the transformProducts
function - that's being compared against the category
variable inside the filterProducts
function. This way, you will be able to leave out the transformProducts
function altogether.
Also, rather than using the OR condition here to compare the category
variable against its original state (empty string), it would be more intuitive if you only apply the filter if a category is selected. Otherwise, render the original set of product items without executing the filter
function at all.
let products = productsData
if (category != "") {
products = productsData.filter[...]
}
[...]
<ProductList products={products} />
[...]
week1/project/ecommerce/src/App.js
Outdated
<div> | ||
<h1>E-commerce App</h1> | ||
<main> | ||
<CategoryList onFilterProducts={setCategory} /> |
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.
The CategoryList
will need some refactoring to meet all requirements of the assignment. It should have the following functionality:
- Render a button/component for each of the categories in the provided fake data file.
- When a category component is clicked: set/unset the selected category and apply a filter on the shown products.
- If a particular category is selected, the corresponding component should be highlighted.
At the moment, your app does #1 and part of #2. I would like you to add the functionality that will un-select a category if already selected and also add the category component highlighting for when a category is selected.
One way of approaching this is by extending the props that you pass to the CategoryList
component to something like the following:
<Categories
categories={allCategories} // this prop is used to render a button for each category
selectedCategory={selectedCategory} // this prop is used for identifying which category is selected and to know what filter to apply to products
onToggle={toggleCategory} // method that selects/de-selects a category
/>
return ( | ||
<div className="product-item"> | ||
<img src={props.image} /> | ||
<h3>{props.title.substring(0, 50)}</h3> |
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.
Adding a hard limit on the product title characters can lead to a somewhat unnatural cut-off of the title. If you need to make sure text fits within a limited space on the product card, I recommend using instead something like the CSS text-overflow
attribute - https://www.w3schools.com/cssref/css3_pr_text-overflow.php
@@ -0,0 +1,23 @@ | |||
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.
This is not needed here.
@@ -0,0 +1,18 @@ | |||
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.
This is not needed here.
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.
Thank you for updating your application, Christina - it now meets all the requirements for week 1, well done!
Enjoy the Christmas break!
const toggleCategory = (value) => { | ||
setCategory(category === value ? "" : value); | ||
}; | ||
|
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.
I like how concise this is, well done!
No description provided.