Evaluate the logic of the project?

Question

React

Good day.

I recently started learning React, my first framework. Decided to implement header first.

In terms of functionality, the menu is excellent. On large screens, a complete list of links to small burgers is displayed. Burger can be closed with a cross, overlay or swipe.





But first of all, I want to ask your opinion on whether I distributed the components correctly.

Boss:

/src/hoc/Layout/Layout.js

import { Component } from 'react';
import Header from '../../components/Navigation/Header/Header';

import '../../libs/fontawesome/css/all.min.css';
import cssClasses from './Layout.module.scss';

class Layout extends Component {
  state = {
    isMenuOpen: false,
  }

  toggleMenuHandler = () => {
    const { isMenuOpen } = this.state;
    this.setState({
      isMenuOpen: !isMenuOpen,
    });
  }

  keyPressHandler = ({ key }) => {
    if (key && key === 'Escape') {
      this.toggleMenuHandler();
    }
  }


  render() {
    const { isMenuOpen } = this.state;

    if (isMenuOpen) {
      document.addEventListener('keydown', this.keyPressHandler)
    } else {
      document.removeEventListener('keydown', this.keyPressHandler)
    }

    return(
      <div className={cssClasses.Layout}>
        <Header isMenuOpen={isMenuOpen} toggleMenuHandler={this.toggleMenuHandler} />
        {this.props.children}
      </div>
    );
  }
}

export default Layout;

Next components:

/src/components/Navigation/Header/Header.js

import Havbar from './Navbar/Navbar';

import cssClasses from './Header.module.scss';

const Header = ({ isMenuOpen, toggleMenuHandler }) => {
  return (
    <header className={cssClasses.Header}>
      <Havbar isMenuOpen={isMenuOpen} toggleMenuHandler={toggleMenuHandler} />
    </header>
  );
};

export default Header;

/src/components/Navigation/Header/Navbar/Navbar.js

import { NavLink, Link } from 'react-router-dom';
import { useSwipeable } from 'react-swipeable';

// UI
import Overlay from '../../../UI/Overlay/Overlay';

import cssClasses from './Navbar.module.scss';
import logo from '../../../../img/logo.jpg';

const links = [
  { to: '/', label: 'Main', exact: true },
  { to: '/faq', label: 'Faq', exact: false },
  { to: '/contacts', label: 'Contacts', exact: false },
];

const navbarLinkGenerator = (linksList, toggleMenuHandler) => {
  return linksList.map(({ to, label, exact }, index) => {
    return (
      <li key={`navlink-${index}`}>
        <NavLink to={to} exact={exact} onClick={toggleMenuHandler} activeClassName={cssClasses.active}>{ label }</NavLink>
      </li>
    );
  });
};

const Havbar = ({ isMenuOpen, toggleMenuHandler }) => {
  const navbarLinksClasses = [cssClasses['Navbar-links']];

  if (isMenuOpen) {
    navbarLinksClasses.push(cssClasses.show);
  }

  const handlers = useSwipeable({
    onSwipedRight: () => toggleMenuHandler(),
  });

  return (
    <>
    <div className="Navbar-logo">
      <div className={cssClasses['Navbar-logo']}>
        <Link to="/">
          <img src={logo} alt="logo" />
        </Link>
      </div>
    </div>

    <div {...handlers} className={navbarLinksClasses.join(' ')}>
    <i className="far fa-times" onClick={toggleMenuHandler}></i>
    <nav>
      <ul>
        {navbarLinkGenerator(links, toggleMenuHandler)}
      </ul>
    </nav>
    </div>

    <div className={cssClasses['Navbar-burger']} onClick={toggleMenuHandler}>
      <i className="fal fa-align-left" />
    </div>
    { isMenuOpen && <Overlay onClick={toggleMenuHandler} /> }
    </>
  );
};

export default Havbar;

/src/components/UI/Overlay/Overlay.js

import classesList from './Overlay.module.scss';

const Overlay = ({ onClick }) => {
  return (
    <div onClick={onClick} className={classesList.Overlay}></div>
  )
};

export default Overlay;

The scss files are not really important.



I will be glad to hear what I did wrong in terms of logic and any other criticism or advice.
Thanks in advance.

Answer 1

You can't do this in render. The place for such a subscription is componentDidMount. I think it would be more correct to subscribe once, and do nothing if isMenuOpen is true.

if (isMenuOpen) {
      document.addEventListener('keydown', this.keyPressHandler)
    } else {
      document.removeEventListener('keydown', this.keyPressHandler)
    }

The rest (the organization of the components) is more of a matter of taste. But if you look at it as a whole, there is no consistency. Somewhere there are components for logical elements, somewhere not.