Interactive e-commerce product page using React, Redux & Sass

Community feedback

• 

  Fazza Razaq Amiarso•2,340

  @fazzaamiarsoPosted over 2 years ago

  Hello there! Great solution overall!

  I found some critical problems in your code.

  • You don't wrap the addEventListener in useEffect. Why it's problematic? the consequence of not attaching event listeners in useEffect is that you can't remove the listener after re-rendering. So, you are effectively attaching new listeners every render, which can slow down performance. Here is my improvement

  useEffect(()=>{
  // must create a separate function so it can be removed
  const resizeFunction = () => {
      if (window.outerWidth <= 768 && isLightBoxOpen) {
        dispatch(toggleLightbox(-1, -1))
      }
    }
   window.addEventListener('resize', resizeFunction)
    
    return () =>  window.removeEventListener('resize', resizeFunction) //cleanup function
  }, [dispatch, isLightBoxOpen])
  

  • Mirrored props in a state. Here is the problematic state.

   const [numberOfProduct, setNumberOfProduct] = useState(props.product.numberInCart)
  

  Why it's a problem? because React won't re-initialize the state when props.product.numberInCart changes in a re-render. So, you are at risk of having a stale state. But, if you just want it to be an initial state, that is not a problem. You can read more on the official docs about this specific problem.

  Hope it's helpful! Goodluck!

  Marked as helpful

  3

  ---

  

  Hikmatullah Mohammadi•60

  @hikmatullah-mohammadiPosted over 2 years ago

  @fazzaamiarso Thank you for your time and tremendous feedbacks. I fixed the first one, and the second one is OK since I use setNumberOfProduct whenever it needs updating.

  Happy coding...

  0
• 

  Kofi Nartey•650

  @kofinarteyPosted over 2 years ago

  Awesome job mate. Here a few changes I figured you could implement. *You seem to have forgotten to set the font to what was specified in the design guide: a simple css import could fix that.

  *On mobile, the nav items do not get hidden and neither do you render the hamburger menu.

  *And it also seems the carousel is stuck to just one form as opposed to the different versions for mobile and desktop.

  Great work all the same. Keep pushing💪

  Marked as helpful

  1

  ---

  

  Hikmatullah Mohammadi•60

  @hikmatullah-mohammadiPosted over 2 years ago

  @kofinartey Thank you for your time and feedbacks.

  • I set the font.
  • The nav is behaving properly, may be there is any issues with your browser.
  • I tried a couple of ways to deal with the carousels properly, but I could not find any good solution for so.

  Happy coding...

  0