React Room Homepage

Community feedback

• 

  Anton•515

  @antaryaPosted about 3 years ago

  Hello Carl,

  Great job, it looks really good.

  Couple ideas/suggestions/improvements for your implementation:

  JS

  [1. The if/else check of window.innerWidth looks redundant you may simplify by:

    // original code
     if (window.innerWidth > 1439) {
        setIsMaxWidth(true);
      } else if (window.innerWidth <= 1439) {
        setIsMaxWidth(false);
      }
    // instead you can write this
    setIsMaxWidth(window.innerWidth > 1439)
  

  [2. From the code, it looks like menuHidden, isMaxWidth, tabs is only used inside specific components, so why have them outside then? My rule is to put things as close as possible where they are needed. Also, moving the tabs variable outside the component will not recreate it on each re-render; it is static data that does not depend on anything.

  The same goes for desktopHeros and mobile mobileHeros. Put it outside of the components. Also, there is no need for those variables as the only thing that is changing is the number that you can get from the slide number.

  HTML

  [3. Where possible, use existing elements, e.g. instead of div to mimic buttons, use the button tag and restyle it as you need. That way, it will retain native browser behaviour. For lists, use ul/li; other elements that can be used are header, nav; take a look at semantic elements and why they are used html-semantics-cheat-sheet.

  CSS

  [4. Regarding responsiveness, it looks strange to see one small column on the desktop when the screen is 1400px in width. You are heavily using fixed width and heights maybe using img tags and max-, min- versions of width/height may help to make it adaptable to other screen sizes.

  [5. While there is nothing wrong with using BEM and CSS modules together, it looks redundant to me. With CSS modules, there is no need to scope class names as they will be already unique. That is, the whole point css.button in one component is different than css.button in another component. Also, you may use some nesting like .about h2 {}. You will get rid of the css[''] style as you can easily find simple/short names for all scoped elements. Take a look at other examples css-modules-examples.

  If you find yourself using a lot of conditions inside className, you may take a look at this library clsx, or classnames.

  React

  [6. As the project's design suggests, there should be other pages; having this in mind even for small projects will help later. For example, having a pages folder with the LandingPage component and moving all related components inside. Or building a general Layout component and using it inside LandingPage as the wrapper.

  [7. In App component window.addEventListener("resize", will be called on every render, which means you are adding more event listeners on each re-render which I guess was not your intention. If you move that code into useEffect(..., []), this will be called only once on the mount, also in useEffect, you can remove the event listener on component unmount using the return method. For inspiration, you may check this library of hooks and particularly react-use which does the same thing as you want.

  [8. In the TopBar component, instead of className conditions everywhere, you may set className on the parent like ...--desktop, and for all that need the same check, use that like ...--desktop .topbar__nav-items inside CSS.

  [9. As HeroImage and PageContent share state and data (as each slider image has different text), you might combine them inside the wrapper component.

  ===

  I hope this will be helpful.

  Keep up the good work!

  Marked as helpful

  1

  ---

  

  Carl Wicker•1,055

  @carlwickerPosted about 3 years ago

  @antarya Thanks for taking the time to look through my code. Really appreciated.

  I'll spend some time going back through my code later today.

  0