omarzikry/ment

eCommerce and Blog site

Hey! Nice to see you.

Welcome to my page!
I'm Omar, Frontend developer from 🇪🇬 Cairo, Egypt, currently living in 🇱🇻 Riga, Latvia.

Things I code with

Where to find me

ment's Issues

Consider having your variables stored at the top of your render method

ment/src/components/UI/Carousel/Carousel.js

Line 133 in b9dc88b

<div className={[classes.text , this.state.currentIndex === index ? classes.activeText : null ].join(' ')} key={image}>

through out the component you are looking up same variables repeatedly. that is not the best approach from a performance point of view.

one good approach is to de-structure your state at the top of the render and use these variables instead of looking up repeatedly

ex :

const {currentIndex, loading, images} = this.state;

Slide doesn't require a separate component

ment/src/components/UI/Carousel/Slide/Slide.js

Line 19 in b9dc88b

const Slide = (props) => {

I think this doesn't require a separate component it just passes props and the structure is fairly simple that it can be used directly in the Carousal component directly.

the translateValue method is confusing

ment/src/components/UI/Carousel/Carousel.js

Line 64 in b9dc88b

translateValue = (currentIndex) => {

the translateValue calculates the translation value and set the state with it. on goToPrevSlide you don't use it to set the translate value and use it on goToNextSlide. so it is not very consistent.

ment/src/components/UI/Carousel/Carousel.js

Line 91 in b9dc88b

this.translateValue(this.state.currentIndex)

1- i think you should unify the way you update your state to avoid confusion during maintenance.
2- considering the asyn nature of react setState I suggest setting both currentIndex & translateValue in the same setState call to make sure both are updated at the same time. the translateValue should just calculate and return the value.

Missing images

./src/components/UI/Card/Card.js

Module not found: Can't resolve '../../../assets/images/Arrow-Right.png' in 'C:\Projects\ment\src\components\UI\Card'

Module not found: Can't resolve '../../../assets/images/Arrow-UP-Right.png' in 'C:\Projects\ment\src\components\UI\Card'

Avoid binding in render

ment/src/components/UI/Navs/NavsLinks/NavsLinks.js

Line 31 in b9dc88b

return <NavsLink id={this.state.categories[key].id} active={this.state.categories[key].id === this.state.selected ? true : null} key={key} click={this.handleClick.bind(this, index , this.state.categories[key])}>{this.state.categories[key].name}</NavsLink>

try to avoid binding inside the render. it is quite expensive when it comes to performance and memory.

an alternative option is to use inline arrow function

instead of

onClick ={this.handleClick.bind(this, arguments)}

you can do

onClick = {()=>{this.handleClick( arguments)}

Refactor the ActiveDots component to functional component

ment/src/components/UI/Carousel/ActiveDots/ActiveDots.js

Line 3 in b9dc88b

class ActiveDots extends Component {

this component is a bit over complicated for its functionality. it doesn't need to be a class component or have an internal state. i did a quick refactor. let me know what do you think?

import React from "react";
import classes from "./ActiveDots.css";

function ActiveDots(props) {
  let { imagesNumber, currentIndex, callIndex } = props;
  const dots = [];
  for (let i = 0; i < imagesNumber; i++) {
    dots.push(
      <p
        key={`dot-no-${i}`}
        className={currentIndex === i ? classes.active : null}
        onClick={() => {
          callIndex(i);
        }}
      >
        {i < 9 ? 0 : ""}
        {i + 1}
      </p>
    );
  }
  return <div className={classes.dots}>{dots}</div>;
}

export default ActiveDots;

Unnecessary component

ment/src/components/Navbar/Navlink/Navlink.js

Line 6 in b9dc88b

<NavLink className={classes.Navlink} to={props.goTo}>{props.children}</NavLink>

this component serves no purpose and just adds another nesting level that just adds unneeded level of complexity. you should just use the <NavLink/> from react-router

never update state directly

ment/src/components/UI/Carousel/ActiveDots/ActiveDots.js

Line 12 in b9dc88b

this.state.imageNumbers.push(counter)

never update state directly. you should use setState to update state to make sure react knows about it and appropriate state is formed.

NEVER mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.

setState() does not immediately mutate this.state but creates a pending state transition. Accessing this.state after calling this method can potentially return the existing value.

There is no guarantee of synchronous operation of calls to setState and calls may be batched for performance gains. setState() will always trigger a re-render unless conditional rendering logic is implemented in shouldComponentUpdate().

If mutable objects are being used and the logic cannot be implemented in shouldComponentUpdate(), calling setState() only when the new state differs from the previous state will avoid unnecessary re-renders.

https://reactjs.org/docs/react-component.html#setstate

why use inline styles here?

ment/src/components/UI/Carousel/Slide/Slide.js

Line 2 in b9dc88b

//import './Slide.css';

why did you choose to go with inline styling here?

This should be split into two components

ment/src/components/UI/Card/Card.js

Line 7 in b9dc88b

const Card = (props) => {

this component is quite big and has a lot of logic for both states. This should be split into two components. one for each state. i think this would be better for readability and maintenance.