Skip to content

liya-w1-react #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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

liya-oz
Copy link

@liya-oz liya-oz commented Jan 28, 2025

No description provided.

@sycons sycons self-requested a review January 31, 2025 13:56
@sycons sycons self-assigned this Feb 2, 2025
Copy link

@sycons sycons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good implementation 👍

  • Copied fake data into src folder as instructed ✅
  • Product list that displays all of the products from the all-products file ✅
  • Category list that displays all of the categories from the all-categories file at the top of the page ✅
  • If the user clicks on a category, only the products that have that category in their category property should be displayed ✅
  • Only 1 category is active at a time and the user should see which category is selected ✅

Requested improvements:

  • The categories are highlighted differently when selected. "Show All" has white text with blue backgroud, the other categories have a dark border which disappears when the user clicks somewhere else. Please make the look for the selected category consistent.
  • The layout of the product cards jumps around when selecting different categories. The number of products in a row can be 4 or 3 or 2. Please improve the product card grid to stay consistent. You can have a look at the sample app to get an idea https://hyf-react-w1-example.netlify.app/
  • Breakdown the app into components.

Additional compliments

  • Nice touch adding a button for '"Show All"
  • Buttons and Product Cards have a visually pleasing border with rounded corners.

import products from "./fake-data/all-products";
import "./App.css";

function App() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move code from App component to other components. If you look at the ecommerce app in the project readme, it has 3 sections: a title, list of categories, list of products. Categories and Products can be their own components and this is the real value of using React.

<div id="root">
<h1>Ecommerce App</h1>

<div className="category-buttons">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this into a component and place it in a folder called components.

))}
</div>

<div className="product-list">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this into a component and place it in a folder called components.

@liya-oz
Copy link
Author

liya-oz commented Feb 3, 2025

Hello @sycons! I completed refactoring. Please take a look. Thank you!

@sycons
Copy link

sycons commented Feb 4, 2025

Great changes. Especially extracting utility functions into a file and creating a hook for category selector. This shows good use of React and adding structrue to your application.

Layout changes look good. The grid stays consistent and responds well to different breakpoints.

Minor recommended changes (but NOT a blocker for approval):

  • Add some vertical spacing between the cards
  • Redeploy to netlify so that your latest changes are there.

@sycons sycons added Approved and removed Needs work labels Feb 4, 2025
@liya-oz
Copy link
Author

liya-oz commented Feb 5, 2025

Minor recommended changes (but NOT a blocker for approval):

  • Add some vertical spacing between the cards
  • Redeploy to netlify so that your latest changes are there.

Thank you @sycons ! I applied your suggestions and added a few more. The app became more practical and nice-looking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants