J
J
Jedi2018-08-22 21:46:20
React
Jedi, 2018-08-22 21:46:20

How to compose a component correctly?

Good night.
I'm new to React, I want you to take a look at my code. I took up the implementation of the "smart menu", I just finished it.
This is the 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;

And here is the menu component ( 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);

As a beginner, it seems to me that this is a mess ... How do you think an experienced react developer would write this particular component?
Did he do checks through the ternary operator inside ? Everything works OK, but the size of the code bothers me a little. How to properly code this component? I will be very glad to your criticism! :) Thanks in advance!render() -> return()

Answer the question

In order to leave comments, you need to log in

1 answer(s)
A
Anton Spirin, 2018-08-22
@PHPjedi

1. Static data. Why are they defined every render? They can be moved outside the component:

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

And in a good way, generate routes and switch pages using the same data:
import routes from './routes';

/* ... */

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

2. By what logic do you use the const and let keywords? Use const for all variables that are not redefined in the code, and let only for those that are redefined. This significantly reduces the cognitive load on the reader of the code.
3. Designs like:
let
  isPrev = currPathIndex !== 0,
  isNext = currPathIndex !== paths.length - 1;

often cause problems when refactoring. It is better not to save on matches and write keywords for each line:
const hasPrev = currPathIndex !== 0;  // (1)
const hasNext = currPathIndex !== paths.length - 1; // (2)

Such code is easier to make changes and does not suffer from errors like a missing comma or a semicolon instead of a comma, which can save time.
isVisible
hasChildren
shouldShowCloseButton

4. If you use a lot of keys from state and props, you can do destructuring: 5. Don't repeat yourself. Try to separate the code that you use repeatedly into separate components:
<Link className="next_btn" style={ this.state.isOpen ? {visibility: 'hidden'} : null} to={paths[currPathIndex + 1].link}>
  <span>{ paths[currPathIndex + 1].title }</span>
</Link>

It can be rewritten by a component with something like this interface:
6. Why ternarks where && is better?
{isOpen && this.renderСloseBtn()} // (1)
{hasPrev && <NavButton prev isVisible={!isOpen} path={prevPath} />}
{hasNext && <NavButton next isVisible={!isOpen} path={nextPath} />}

Use the ternary operator in render where there is an alternative:
But don't use nested:
Better this way:
where this.renderUserMenu is:
renderUserMenu() {
  return this.props.isMobile ? <UserMenuMobile /> :  <UserMenuDesktop />;
}

7. Try not to write redundant constructions:
class Menu extends Component {
  constructor(props) {
    super(props);
      this.state = {
          isOpen: false,
      };
  }
}

When using the stage-0 presset, create-react-app, jsfiddle and other libraries/boilerplates/services that use the babel-plugin-transform-class-properties plugin, it is enough:
class Menu extends Component {
  state = {
     isOpen: false,
  };
}

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question