Skip to content
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

Recommended code improvments #1

Open
rksh1997 opened this issue Feb 6, 2021 · 1 comment
Open

Recommended code improvments #1

rksh1997 opened this issue Feb 6, 2021 · 1 comment

Comments

@rksh1997
Copy link

rksh1997 commented Feb 6, 2021

I would like to suggest the following code improvments:

  • Follow one file naming convention, I see that you are using 3, Capiltalized like AboutPage, Snake case like products_actions and Kabab case like about-usbanner.jpg.
  • You are directly doing http requests inside the actions. This makes the API coupled to redux. What if you some time in the future needed to fetch the products without calling a redux dispatch? It's better to separate actions from http calls.
  • It would be much more readable if you put the action type above the action itself, for example:
export const FETCH_PRODUCTS = 'FETCH_PRODUCTS'
export function fetchProducts = () => {
  return async () => ...
}
  • Some components are directly depending on redux. For example AllProducts gets the loading state from redux store directly. What if you wanted to replace redux with something else in the future ? you will have to visit all these components and change them. Instead, get the loading state from props and only connect the page itself (the container) and not the component (the representational one).
  • You have some repeated code like
<section className="py-5">
        <div className="container">
          <Title title="BEST SELLING" />
          <div className="row">

once for loading and once to show products. Why not using the conditional rendering ?

  • You have some static css (not going to chage) but being passed as a style object to the component like
<div
          style={{ textAlign: 'center' }}
          className="col-10 mx-auto pt-3"
>

This is going to re-render the component in every update although it might not need to be re-rendered.

  • I suggest doing some components for smaller elements like the button and the input so that you don't have to write css classes again again.
  • There are some useless comments like this one
// import navbar links from utils
import { navbarLinks } from '../../utils/navbarLinks';
  • A component is either representational or connected, mixing both in one component is not good.

However, the code overall is actually good and you are great at styling. best of luck :D and sorry for annoying

@IbrahiimKamal
Copy link
Owner

thanks so much

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

No branches or pull requests

2 participants