Skip to content

Vlada_Bessikalo-w1-React #5

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

Conversation

VladaBessikalo
Copy link

@ChingHsun ChingHsun self-assigned this Feb 7, 2025
@ChingHsun
Copy link

Hi Vlada, Your code works well and shows a good understanding of React. Great work!
I left some comments and a minor change, but it is not a blocker for approval.

Assignment Requirements ✅

  • A product list that displays all of the products in the all-products file.
  • the site is responsive, so have a look at the breakpoints in the deployed example project.
  • A category list that displays all of the categories in 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 on the screen.
  • The categories listed in the product objects do not match up exactly with the categories in the categories list. You will have to find a solution to this without editing the files
  • There should only be 1 category active at a time and the user should see which category is selected.
  • You need to deploy your app somewhere (using something like netlify) and put the link in your PR!

Comments Rules

Type Description
Suggestions • Common best practices, standard improvements
• Should be considered for implementation
Nice-to-have • Based on personal preferences or coding style
• Optional enhancements
Question • Starting a discussion to understand technical decisions
• Asking for clarification or exploring different solutions
Praise • Recognizing and highlighting good practices
• Encouraging positive patterns

Suggestions 💡

  • All in comments

Nice-to-have ✨

  • Over-componentization: Small, single-purpose pieces of code that are only used once don't need to be split into separate components. Since it is a small assignment, I will only use Categories and ProductItem.

Praise 🌟

  • Using ESLint for consistent code style
  • Great Git Workflow: Clear, descriptive commit messages, easy to follow your development process
  • Clean Architecture, Well-organized component structure
  • Logical data flow from App to child components
  • Strong Naming Conventions and prop names
  • Descriptive class names and Self-documenting code

Nice one!

@@ -0,0 +1,26 @@
/* eslint-disable react/prop-types */

Choose a reason for hiding this comment

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

Suggestion:
Since you don't want this rule in the whole project, you can turn it off in the eslint config

};

const normalizeCategory = (category) => {
return category.replace('FAKE:', '').trim();

Choose a reason for hiding this comment

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

Suggested change
return category.replace('FAKE:', '').trim();
return category.replace('FAKE: ', '');

Suggestion:
you don't need trim

setSelectedCategory(category);
};

const normalizeCategory = (category) => {

Choose a reason for hiding this comment

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

Suggestion:
This function can be handled in the Categories component.
Due to the Single Responsibility principle, the App pages shouldn't need to know the 'FAKE:' details.

Choose a reason for hiding this comment

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

Suggestion:
Removing unused files that came with the project template. such as index.css, and assets folder

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