Skip to content

w1-assignment #2

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

Conversation

Dalia-Saeed
Copy link

No description provided.

@sycons sycons self-requested a review January 31, 2025 13:57
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.

I left some questions and a minor change, but it is not a blocker for approval.

title: PropTypes.string.isRequired,
image: PropTypes.string.isRequired,
})
).isRequired,
Copy link

Choose a reason for hiding this comment

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

Out of curiousity, what is the value of defining prop types for this component? What happens if it is not defined?

@sycons sycons self-assigned this Feb 2, 2025
@sycons
Copy link

sycons commented Feb 4, 2025

This requirement to deploy the app is missing from the PR. Can you add it please?

You need to deploy your app somewhere (using something like netlify) and put the link in your PR!

@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.

3 participants