Оцените логику проекта?

Question

[Ivan Ivanovich]

Доброго времени суток.

Недавно начал изучать React, мой первый фреймворк. Решил для начала реализовать header.

В плане функциональности меню отличное. На больших экранах отображается полный список ссылкок, на мелньких бургер. Бургер можно закрыть по крестику, оверлею или свайпом.





Но я в первую очередь хочу спросить ваше мнение по поводу того, правильно ли я распределил компоненты.

Босс:

/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;

Далее компоненты:

/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;

Файлы scss думаю не особо важны.



Буду рад услышать, что я сделал не правильно в плане логики и любую другую критику или советы.
Заранее спасибо.

Answer 1

[twolegs]

В render такое делать нельзя. Место для такой подписки - componentDidMount. Думаю, правильнее будет подписаться один раз, и не делать ничего, если isMenuOpen в true.

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

Остальное (организация компонент) - это скорее вкусовщина. Но если смотреть в целом - нет какой-то консистентности. Где-то есть компоненты для логических элементов, где-то нет.

Comments

[Ivan Ivanovich]

twolegs, спасибо, учту.

[Ivan Ivanovich]

twolegs, реализовал такой вариант:

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

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

Answer 2

[n1ksON]

1. Зачем смешиваете классовые и функциональные компоненты?
2. Вынесите эти участки кода в отдельные файлы

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 links = [
  { to: '/', label: 'Main', exact: true },
  { to: '/faq', label: 'Faq', exact: false },
  { to: '/contacts', label: 'Contacts', exact: false },
];

3. Лучше переписать так:
было:

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

UPD:
4. Лучше переписать с useEffect:

useEffect(() => {
  if (isMenuOpen) {
    navbarLinksClasses.push(cssClasses.show);
  }
}, [isMenuOpen])

Comments

[Ivan Ivanovich]

n1ksON, спасибо, исправлю

[Ivan Ivanovich]

n1ksON, подскажите пожалуйста, нужно ли мне при отделении navbarLinkGenerator в отдельный файл создавать для него отдельную папку, т.е. будет так:
src/components/Navigation/Header/Navbar/NavbarLinkGenerator/NavbarLinkGenerator.js
или можно прямо в папку с Navbar закинуть:
src/components/Navigation/Header/Navbar/NavbarLinkGenerator.js ?
Я предполагаю что корректнее будет первый вариант с отдельной папкой, но прав ли я?

[n1ksON]

Ivan Ivanovich, конечно лучше с отдельной папкой.
Ещё обновил и добавил 4 пункт

[Ivan Ivanovich]

n1ksON, спасибо большое