Прокомментируете тестовое на react?

Question

[Aleksandr Aladin]

Коллеги, привет!

Нахожусь в поиске работы на react стэке, а так как коммерческого опыта на react нет, то делаю для тренировки тестовые. Последнее от aviasales. Понимаю, что в нем нет ничего особенно сложного, но все равно хотелось бы услышать ваше мнение относительно кода и в целом относительно организации папок и файлов, узнать, что бы вы улучшили или изменили(не считая добавления state-менеджера и тестов).
Код
Демо

Заранее спасибо, буду благодарен за любой фидбэк.

Comments

[Алексей Лауд]

Не претендую на полный комментарий по этому заданию, но то, что точно можно улучшить:
Вместо передачи данных и коллбеков от компонента к компоненту по цепочке, лучше использовать что-то вроде Redux.

[Афанасий Захаров]

Все в целом хорошо, на мой взгляд, есть правда один момент, который лично мне в глаза бросился

componentDidMount() {
    this.setState({
      isLoading: true
    });
    api.getTickets()
      .then(({ tickets }) => {
        const sortedTickets = tickets.sort((ticket_1, ticket_2) => ticket_1.price - ticket_2.price);
        const stops = {
          [sortedTickets[0].stops]: sortedTickets[0].stops
        };
        const maxStops = sortedTickets.reduce((acc, currValue) => {
          stops[currValue.stops] = currValue.stops;
          return acc.stops > currValue.stops ? acc : currValue;
        }).stops;

        if (Object.keys(stops).length - 1 === maxStops) {
          stops.all = 'all';
        }

        this.setState({
          isLoading: false,
          ticketsWithCurrentCurrency: sortedTickets,
          tickets: sortedTickets,
          ticketsFilteredByStops: sortedTickets,
          stops,
          maxStops
        });
      })
      .catch(() => {
        this.setState({
          isLoading: false,
          error: 'Something went wrong<br>Please try again later'
        });
      });
}

Вроде как таких моментов там несколько, и я бы посоветовал в componentDidMount сделать просто вызов метода, например getTickets, в котором намного логичнее хранить логику обработки билетов.

[albertalexandrov]

В итоге взяли? Сам хочу к ним податься..

[Aleksandr Aladin]

albertalexandrov, Я его делал без цели попасть в aviasales) Просто решил применить изученный материал по react.

[albertalexandrov]

Aleksandr Aladin, понятно)

Answer 1

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

1. Хотелось бы видеть в проекте использование redux. react+redux - это самый распространенный и востребованный стек в React разработке.

2. Почему все хандлеры и состояния находятся в App, а не в Main? Как вы потом эту кашу собираетесь масштабировать? Переносите все, что связанно только с Main в Main. По-хорошему смотрите пункт 1.

3. Слишком много функциональных компонентов. Подумайте их где можно заменить на классы с реализованным shouldComponentUpdate или на PureComponent, чтобы убрать лишние вызовы render этих компонентов.

4. import Logo from 'images/Logo.png';
называть пути к ресурсам с заглавной буквы неправильно.

5. Вместо:

const StyledLogo = styled.img.attrs({
  src: Logo,
  alt: 'Aviasales'
})`
  width: 60px;
  height: 61px;
`;

Удобней в использовании:

const StyledLogo = styled.img`
  width: 60px;
  height: 61px;
`;

и:
<StyledLogo src={logo} alt="Aviasales" />

6.

const Error = ({ text }) => (
  <StyledError dangerouslySetInnerHTML={{__html: text}} />
);

зачем тут html?
Для сохранения переносов строки есть css правило:
white-space: pre-line;

7. Вместо:

let element;

if (error && !isLoading) {
  element = <Error text={error} />;
}
if (!error && isLoading) {
  element = <Loader />;
}
if (!error && !isLoading) {
  element = (
    <>
    <Heading />
    <Main
    isCurrencyExchanging={isCurrencyExchanging}
    activeCurrency={activeCurrency}
    handleCurrencyChange={this.handleCurrencyChange}
    ticketsFilteredByStops={ticketsFilteredByStops}
    stops={stops}
    handleStopsChange={this.handleStopsChange}
    handleUncheckOther={this.handleUncheckOther}
    />
    </>
  );
}
return element;

Лучше:

if (isLoading) return <Loader />;

if (error) return <Error text={error} />;

return (
  <>
    <Heading />
    <Main
      isCurrencyExchanging={isCurrencyExchanging}
      activeCurrency={activeCurrency}
      handleCurrencyChange={this.handleCurrencyChange}
      ticketsFilteredByStops={ticketsFilteredByStops}
      stops={stops}
      handleStopsChange={this.handleStopsChange}
      handleUncheckOther={this.handleUncheckOther}
    />
  </>
);

8. Вместо:

filterTickets = (tickets, stops) => {
  return tickets.filter((ticket) => {
    return values(stops).indexOf(ticket.stops) !== -1;
  });
};

Лучше:

filterTickets = (tickets, stops) => tickets.filter(
  ticket => values(stops).includes(ticket.stops),
);

9. Не пропускайте отступы между методами и между вложенными свойствами css.

10. Вместо:

componentsDidMount() {
  // много кода
}

Лучше:

componentsDidMount() {
  this.fetchSomeData();
}

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

12. Loader и Error самое место в директории components/core или что-то вроде того. Там же, по-хорошему, должны находиться базовые компоненты: кнопки, инпуты, табы, чекбоксы.

13. Styled компоненты, имхо, лучше писать в файле с компонентом, где они применяются. Так анализ кода происходит гораздо быстрей и легче поддерживать. Исключение - переиспользуемые компоненты.
Даже если вам больше нравится выносить, называть файл style неправильно, вы там описываете компоненты, а не просто стили.

Comments

[Aleksandr Aladin]

Супер, спасибо!

[Макс]

По 10-му пункту: injectGlobal deprecated, см. createGlobalStyle

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

Maxim, не знал, так как на старой версии сижу. С новым проектом изучу вопрос. Поправил. Спасибо.