A
A
Alexander Aladin2018-10-15 16:22:59
Frontend
Alexander Aladin, 2018-10-15 16:22:59

Can you comment on the test in react?

Colleagues, hello!
I am looking for work on the react stack, and since there is no commercial experience on react, I do test ones for training. Latest post from aviasales. I understand that there is nothing particularly complicated in it, but I would still like to hear your opinion on the code and in general regarding the organization of folders and files, to find out what you would improve or change (not counting the addition of a state manager and tests).
Code
Demo
Thanks in advance, any feedback would be greatly appreciated.

Answer the question

In order to leave comments, you need to log in

1 answer(s)
A
Anton Spirin, 2018-10-15
@sanchos86

1. I would like to see the use of redux in the project. react+redux is the most widely used stack in React development.
2. Why are all handlers and states in App and not in Main? How are you going to scale this mess? Move everything related only to Main to Main. In a good way, see point 1.
3. Too many functional components. Think about where you can replace them with classes with shouldComponentUpdate implemented or with PureComponent to remove unnecessary render calls for these components.
4. Capitalizing resource paths is wrong. 5. Instead of:import Logo from 'images/Logo.png';

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

More convenient to use:
const StyledLogo = styled.img`
  width: 60px;
  height: 61px;
`;

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

why is there html?
To save line breaks there is a css rule:
7. Instead of:
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;

It is better:
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. Instead of:
filterTickets = (tickets, stops) => {
  return tickets.filter((ticket) => {
    return values(stops).indexOf(ticket.stops) !== -1;
  });
};

It is better:
filterTickets = (tickets, stops) => tickets.filter(
  ticket => values(stops).includes(ticket.stops),
);

9. Don't skip padding between methods and nested css properties.
10. Instead of:
componentsDidMount() {
  // много кода
}

It is better:
componentsDidMount() {
  this.fetchSomeData();
}

11. Directories and index files for each component, IMHO, are superfluous. It is better to define components in a file of the same name, and only when it becomes necessary to decompose it, replace it with a directory and index.
12. Loader and Error should be in the components/core directory or something like that. In the same place, in a good way, there should be basic components: buttons, inputs, tabs, checkboxes.
13. Styled components, IMHO, it is better to write in a file with a component where they are applied. This makes code analysis much faster and easier to maintain. The exception is reusable components.
Even if you like to render more, naming the style file is wrong, you are describing components there, not just styles.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question