Нормально ли написаны компоненты??

Question

[AlexKindGeek]

Привет. Ребят, необходим взгляд со стороны на мой код.
Нормально ли написаны компоненты??
Возможно есть моменты, на которые стоит обратить внимание?

UserListComponent

import React, { Component, Fragment } from "react";
import { connect } from "react-redux";

/**Components */
import InfiniteScroll from "react-infinite-scroll-component";
import { BeatLoader } from "react-spinners";
import User from "./User";

/**Actions */
import { getListOfUsers, clearData, setUserChat } from "../../../actions";

/**Configs, Utils */

/**Material UI */

class UserList extends Component {
  state = {
    per_page: 15
  };

  componentDidMount() {
    this.getChatsUser();
  }

  /**Called when we change chat type (all, unread, read etc.) */
  componentDidUpdate(prevProps) {
    const { clearData } = this.props;
    if (prevProps.params !== this.props.params) {
      clearData();
      this.getChatsUser();
    }
  }

  fetchMoreData = () => {
    this.getChatsUser();
  };

  /**
   * Get query of params from props and called api action for getting users.
   * When fetchMoreData is called, we should increase per_page and
   * override userData at reducer (getListOfUsers action)
   */
  getChatsUser = () => {
    const { getListOfUsers, params } = this.props;
    const { per_page } = this.state;
    const query = Object.assign({}, params, { per_page });
    getListOfUsers(query).then(() => {
      this.setState({
        per_page: this.state.per_page + 5
      });
    });
  };

  render() {
    const {
      chatReducer: { userData },
      setUserChat
    } = this.props;
    return (
      <Fragment>
        <InfiniteScroll
          dataLength={userData.length}
          next={this.fetchMoreData}
          hasMore={true}
          height={520}
          loader={
            <BeatLoader
              className="chat-loader"
              sizeUnit="px"
              size={12}
              color="#123abc"
            />
          }
          endMessage={
            <p style={{ textAlign: "center" }}>
              <b>Yay! You have seen it all</b>
            </p>
          }
        >
          {userData.map((row, index) => (
            <User setUserForChat={setUserChat} key={row.id} userData={row} />
          ))}
        </InfiniteScroll>
      </Fragment>
    );
  }
}

const mapDispatchToProps = {
  getListOfUsers,
  clearData,
  setUserChat
};

const mapStateToProps = (state) => ({
  chatReducer: state.chatReducer
});

export default connect(
  mapStateToProps,
  mapDispatchToProps
)(UserList);

User Component

import React from "react";
import moment from "moment";

class User extends React.Component {
  formatTime = (data) => {
    if (data) {
      const started = moment.unix(data).format("HH:mm MM/DD/YYYY");
      const finished = moment().format("HH:mm MM/DD/YYYY");
      let minutes = moment(finished).diff(moment(started), "minutes");
      const days = moment(finished).diff(moment(started), "days");
      const hours = Math.floor(minutes / 60);
      minutes %= 60;
      if (days > 0) {
        return moment.unix(data).format("MM/DD/YYYY");
      }
      if (hours > 0) {
        return `${hours}h`;
      }
      return `${minutes}m`;
    }
  };

  render() {
    const {
      userData,
      userData: { user_id, message_created_at, message, unread_amount },
      setUserForChat
    } = this.props;
    return (
      <div
        className="user-data-container"
        onClick={() => setUserForChat(userData)}
      >
        <div className="user-cell-container">
          <div className="user-id-container">User # {user_id}</div>
          <div className="message-time-container">
            {this.formatTime(message_created_at)}
          </div>
        </div>
        <div className="user-cell-container">
          <div className="last-message-container">
            {message || "No messages"}
          </div>
          {unread_amount > 0 ? (
            <div className="unread-message-container">{unread_amount}</div>
          ) : (
            ""
          )}
        </div>
      </div>
    );
  }
}

export default User;

Answer 1

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

1. formatTime - хелпер, я не буду таратить время на анализ качества его реализации, скажу лишь, что в компоненте ему не место. Перенесите в папку lib, utils или как там она у вас называется, импортируйте где нужен и используйте.

2. Закроем, пока глаза на пункт 1. formatTime у вас написан стрелочной функцией, но он никуда не передается и в нем не используется ключевое слово this - определять его стрелочной функцией было бессмысленно. Изучите вопрос и постарайтесь понять зачем их используют.

spoiler

привязка контекста

То же с getChatsUser - функция никуда не передается.

3. Никогда не используйте тернарки если, альтернативный кейс null. Замените на:

render() {
  /* ... */
  const hasUnreadMessages = unread_amount > 0;
  
  return (
    <div>
      {hasUnreadMessages && (
        <div className="unread-message-container">{unread_amount}</div>
      )}
    </div>
  );
}

4. Зачем передаете в mapStateToProps все состояние редьюсера? Передавайте только необходимые компоненту данные:

export const userDataSelector = state => state.chatReducer.userData;

const mapStateToProps = state => ({
  userData: userDataSelector(state),
});

Селекторы очень полезная вещь. В реальных приложениях они часто помногу раз переиспользуются и в случае изменения структуры store, вам придется изменить только селекторы, вместо изменения реализаций mapStateToProps по всему приложению. Так же, они, зачастую, короче.

Почитайте про библиотеку reselect.

5. Вместо:

const query = Object.assign({}, params, { per_page });

лаконичней и проще для анализа:
const query = { ...params, per_page };

6. Правильное обновление состояния на основе предыдущего:

this.setState(prevState => ({
  per_page: prevState.per_page + 5,
}));

7. Использование trailing comma - очень хорошая практика, советую ей не пренебрегать.

8. Более оптимизированным вариантом для списков, будет вместо определения стрелочных функции в элементах каждый render:
<div onClick={() => setUserForChat(userData)}>
использовать data-атрибуты:

<div data-id={userData.id} onClick={setUserForChat}>

Реализацию setUserForChat придется изменить:

setUserForChat = e => {
  const { id } = e.target.dataset;
  // some code with using id
}

Comments

[AlexKindGeek]

Спасибо за ответ:
1, 2, 3, 5, 6, 7- понял, буду исправлять
4 - возможно надо будет и другие данные редюсера
8 - setUserForChat это action. Не очень хочу этот компонент оборачивать в connect что-бы в mapDispatchToProps передать, поэтому передал его с родителя

export const getListOfUsers = (params) => (dispatch) => {
  return Api.post(`${domen.users}${api.chatUserList}`, params).then(
    ({ data }) => {
      dispatch(setUserData(data));
    }
  );
};

export const setUserData = (payload) => {
  return {
    type: `${T.CHAT}_SET_USERS`,
    payload
  };
};

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

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

Answer 2

[abberati]

хороший тон - в тернарных операторах последним операндом вместо "" ставить null