Как составить компонент грамотно?

Question

[Jedi]

Ночка добрая.

Я новичок в React, хочу чтобы вы посмотрели мой код. Я взялся за реализацию "умного меню", только что дописал.

Это App Component (App.js):

import React, { Component } from 'react';
import { Switch, Route } from 'react-router-dom';
import './App.css';
import Menu from "./components/menu/Menu";
import Home from "./components/pages/home/Home";
import Contacts from "./components/pages/contacts/Contacts";
import About from "./components/pages/about/About";
import Projects from "./components/pages/projects/Projects";

class App extends Component {
  render() {
    return (
      <div className="App">
        <Menu/>
        <main>
          <Switch>
            <Route exact path="/" component={Home}/>
            <Route path="/about" component={About}/>
            <Route path="/projects" component={Projects}/>
            <Route path="/contacts" component={Contacts}/>
          </Switch>
        </main>
      </div>
    );
  }
}

export default App;

А здесь именно компонент меню (Menu.js):

import React, { Component } from 'react';
import { Link, withRouter } from 'react-router-dom';
import './Menu.css';

class Menu extends Component {
    constructor(props) {
        super(props);
        this.state = {
            isOpen: false,
        };
    }

    componentWillMount() {
        this.props.history.listen(() => {
            // view new URL
            console.log('New URL', this.props.history.location.pathname);
        });
    }

    render() {
        const paths = [
            {
                title: 'Главная',
                link: '/'
            },
            {
                title: 'О нас',
                link: '/about'
            },
            {
                title: 'Проекты',
                link: '/projects'
            },
            {
                title: 'Контакты',
                link: '/contacts'
            },
        ];

        let
            currPath = this.props.history.location.pathname,
            currPathIndex = paths.findIndex(n => n.link === currPath);

        let
            isPrev = currPathIndex !== 0,
            isNext = currPathIndex !== paths.length - 1;

        let closeBtn = <Link className="close-btn" onClick={ () => {this.setState({isOpen: !this.state.isOpen})} } style={ this.state.isOpen ? {visibility: 'visible', opacity: '1', zIndex: '2'} : {visibility: 'hidden', opacity: '0'} } to="#"/>;

        return (
            <div>
                <nav>
                    <a className="menu-btn" onClick={ () => {this.setState({isOpen: !this.state.isOpen})} } style={ this.state.isOpen ? {visibility: 'hidden'} : null}>
                        <span>Меню</span>
                    </a>
                    { this.state.isOpen ? closeBtn : null }
                    {isNext ? (
                        <Link className="next_btn" style={ this.state.isOpen ? {visibility: 'hidden'} : null} to={paths[currPathIndex + 1].link}>
                            <span>{ paths[currPathIndex + 1].title }</span>
                        </Link>
                    ) : null}
                    {isPrev ? (
                        <Link className="prev_btn" style={ this.state.isOpen ? {visibility: 'hidden'} : null} to={paths[currPathIndex - 1].link}>
                            <span>{ paths[currPathIndex - 1].title }</span>
                        </Link>
                    ) : null}
                </nav>

                <div className="menu_frame" style={ this.state.isOpen ? {opacity: '1', zIndex: '2', transition: 'opacity 750ms ease 0s'} : {opacity: '0', zIndex: '-1', transition: 'all 500ms ease 0s' }}>
                    menu is opened.

                    <br/>

                    <strong>Route is:</strong> <i>{ this.props.history.location.pathname }</i>
                </div>
            </div>
        );
    }
}

export default withRouter(Menu);

Как новичку, мне кажется, что это каша... Как Вы думаете, как бы написал опытный реакт-разработчик именно этот компонент?

Делал ли он проверки через тернарный оператор внутри render() -> return() ?

Все работает ОК, но величина кода меня немного напрягает.

Как грамотно составить код этого компонента?

Буду очень рад вашей критике! :)

Заранее спасибо!

Answer 1

[Антон Спирин]

1. Статические данные. Зачем они определяются каждый render? Их можно вынести за пределы компонента:

const paths = [ ... ];
class Menu extends Component { ... }

А по-хорошему генерировать роуты и переключать страницы по одним и тем же данным:

import routes from './routes';

/* ... */

<Switch>
  {routes.map(route => (
    <Route
      exact={route.isExact}
      path={route.path}
      component={route.component}
    />
  )}
</Switch>

2. По какой логике используете ключевые слова const и let? Используйте const для всех переменных которые не переопределяются в коде, а let только для тех, которые переопределяются. Это заметно снижает когнитивную нагрузку с читающего код.

3. Конструкции вроде:

let
  isPrev = currPathIndex !== 0,
  isNext = currPathIndex !== paths.length - 1;

часто вызывают проблемы при рефакторинге. Лучше не экономить на спичках и писать ключевые слова для каждой строки:

const hasPrev = currPathIndex !== 0;  // (1)
const hasNext = currPathIndex !== paths.length - 1; // (2)

В такой код проще вносить изменения и ему не страшны ошибки вроде пропущенной запятой или точки с запятой вместо запятой, что может сэкономить время.

(1), (2) - обратите внимание, что я заменил префикс is на has, что правильней. Для булевых переменных помимо is, можно использовать следующие префиксы is, has, should и подобные. Примеры:

isVisible
hasChildren
shouldShowCloseButton

4. Если используете много ключей из state и props можно сделать деструктуризацию:
const { isOpen } = this.state;

5. Don't repeat yourself. Код который вы используете многократно старайтесь выносить в отдельные компоненты:

<Link className="next_btn" style={ this.state.isOpen ? {visibility: 'hidden'} : null} to={paths[currPathIndex + 1].link}>
  <span>{ paths[currPathIndex + 1].title }</span>
</Link>

Его можно переписать компонентом с примерно таким интерфейсом:

<NavButton prev isVisible={!isOpen} path={prevPath} />

6. Зачем тернарки там где лучше подойдет &&?

{isOpen && this.renderСloseBtn()} // (1)
{hasPrev && <NavButton prev isVisible={!isOpen} path={prevPath} />}
{hasNext && <NavButton next isVisible={!isOpen} path={nextPath} />}

(1) - обратите внимание, что рендер кнопки вынесен в отдельный метод, так она будет создаваться только тогда, когда она нужна. В вашем же коде она определяется даже тогда когда не добавляется на страницу.

Используйте тернарный оператор в render, там где есть альтернативный вариант:

{isFetching ? <Preloader /> : <Content data={data} />}

Но не используйте вложенных:

{isSignedIn ? isMobile ? <UserMenuMobile /> :  <UserMenuDesktop /> : <MainMenu /> }

Лучше так:

{isSignedIn ? this.renderUserMenu() : <MainMenu /> }

где this.renderUserMenu:

renderUserMenu() {
  return this.props.isMobile ? <UserMenuMobile /> :  <UserMenuDesktop />;
}

7. Старайтесь не писать избыточных конструкций:

class Menu extends Component {
  constructor(props) {
    super(props);
      this.state = {
          isOpen: false,
      };
  }
}

При использовании прессета stage-0, create-react-app, jsfiddle и прочих библиотек/бойлерплейтов/сервисов, использующих плагин babel-plugin-transform-class-properties, достаточно:

class Menu extends Component {
  state = {
     isOpen: false,
  };
}

Comments

[Jedi]

Вау! Низкий поклон Вам за сколь подробный разбор. Спасибо!

Answer 2

[0xD34F]

Как Вы думаете, как бы написал опытный реакт-разработчик...

Я конечно таковым ни разу не являюсь, но всё-таки скажу пару слов.

проверки через тернарный оператор внутри render?

Это окей - conditional rendering.

render() {
    const paths = [

Из метода render массив paths следует убрать. Может быть, его даже стоит сделать параметром компонента.

onClick={ () => {this.setState({isOpen: !this.state.isOpen})} }

Следует сделать метод. Создавать при каждом вызове render новый обработчик - само по себе не очень здорово, ну и ещё этот код повторяется дважды.

this.props.history.listen(...

Если предполагается, что во время работы приложения экземпляр компонента будет создаваться несколько раз, то при удалении экземпляра надо снимать обработчик, иначе вывод в консоль задвоится (затроится, зачетверится,...). Сделать это несложно - listen возвращает функцию, снимающую обработчик, так что:

componentWillMount() {
  this.unlisten = this.props.history.listen(...

componentWillUnmount() {
  this.unlisten();

<Link className="next_btn"...
<Link className="prev_btn"...
Выглядят подозрительно похожими. Я бы рассмотрел вариант сделать под них отдельный компонент, который бы получал isOpen и свойства объектов paths[currPathIndex +/- 1] в качестве параметров.

Comments

[Jedi]

Большое при большое Вам Спасибо!