Skip to content

Esen karatas w1 react #8

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 9 commits into
base: main
Choose a base branch
from

Conversation

esenkaratas
Copy link

No description provided.

@sycons sycons self-requested a review January 31, 2025 13:57
@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 ✅
  • Page is reponsive and adapts to various breakpoints ✅

Requested improvements:

  • Please deploy app to netlify and and the link to the PR description.
  • There is a bullet next to each category. Please remove it to improve the layout. It looks like the result of using the <li> tag. Maybe a different html tag is better suited for this.
  • There isn't a way to return to all products after selecting a category. Please add functionality so that when a category is clicked again, it will remove the filtering and show all products.
  • Move components into their own files. This adds structure to the application and makes improves maintainability, instead of having one big App.jsx file that keeps growing.

Additional compliments

  • Nice layout! Really love the product cards being inside a card with rounded corners and the slight enlarge effect when hovering. Great styling work 👏

export default function App() {
const [activeCategory, setActiveCategory] = useState("all");

function normalizeCategory(category) {
Copy link

Choose a reason for hiding this comment

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

Great use of extracting reusable logic into a function 👏

);
}

function Header() {
Copy link

Choose a reason for hiding this comment

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

Please move this component into it's own file and place it in a folder called components.

);
}

function FilteredCategories({
Copy link

Choose a reason for hiding this comment

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

Please move this component into it's own file and place it in a folder called components.

setActiveCategory: PropTypes.func.isRequired,
};

function ProductList({ products }) {
Copy link

Choose a reason for hiding this comment

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

Please move this component into it's own file and place it in a folder called components.

products: PropTypes.array.isRequired,
};

function ProductItem({ product }) {
Copy link

Choose a reason for hiding this comment

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

Please move this component into it's own file and place it in a folder called components.

@esenkaratas
Copy link
Author

Here’s the live version of the app: https://esens-ecommerce-app.netlify.app

@sycons
Copy link

sycons commented Feb 5, 2025

All requested changes have been implemented.
The layout looks great and the app is now better structured and takes advantage of reusable components 👍
Good job!

I went through the code locally to complete the review, to work around the number of changed files being too high to review it in GitHub.

@sycons sycons added Approved and removed Needs work labels Feb 5, 2025
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