Skip to content

Liya-w2-react #21

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

Conversation

liya-oz
Copy link

@liya-oz liya-oz commented Feb 5, 2025

No description provided.

@sycons sycons self-assigned this Feb 8, 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.

Hi Liya, there are 2 versions for src folder and package.json file in this PR. One is in the project folder and the other is in the ecommerce folder. Basically there are 2 React Apps in this PR. Can you clean this up so that there is only 1 React App and I can continue the review on that app?

Can you also add the link to the deployed app (somewher like netlify) to this PR?

Thank you.

@liya-oz
Copy link
Author

liya-oz commented Feb 11, 2025

Hi @sycons!

I made the changes you requested:

  • Removed one of the React apps and kept just necessary one
  • Added the link to the deployed app on Netlify

Please have a look! Thank you!

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.

Following was implemented:

  • fake-data directory is no longer used ✅
  • App connects to endpoints in the API to fetch data for categories and products ✅
  • Filter the products in the API and not in the frontend ✅
  • App displays loading spinner when waiting for request to complete fetching data ✅
  • App shows an error message if the request failed ✅
  • Clicking on product card goes to a detail page ✅
  • Detail page fetches data from an endpoint ✅
  • Deploy app somewhere (like netlify) and add link to PR ✅

Compliments:
The stlying of this app is sleek and smooth. It responds beautifully for different endpoints, with the padding and the layout holding really well ⭐

Some optional changes have been suggested. These are not a blocker for approval.

Only the change requested in App.jsx is required to approve this PR.

@liya-oz
Copy link
Author

liya-oz commented Feb 25, 2025

Hi @sycons! 😊
Thank you so much for your feedback! I've implemented both optional changes and the updates in App.jsx.

My eCommerce app (Week 2 project) is live on Netlify.

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.

Amazing work Liya! ⭐

I appreciate you implementing even the optional changes. The code is a lot more polished and well structured now.

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