FL11 React / JSX Homework feedback

FL11_React_Feedback.md

Critical

The application contains critical errors that lead to application crash

Direct state modification. The state should be updated via this.setState function, don't update state by using dirrect assignment or modification.

Bad

componentDidMount() {
  this.state.emoji = this.state.emoji.filter(item => item.id !== id);
  //                      ^ = No dirrect assignemnt!R
}

Good

componentDidMount() {
  this.setState(prevState => ({
    emoji: prevState.emoji.filter(item => item.id !== id);
  }))
}

Bad

componentDidMount() {
  this.state.emoji.push({})
               // ^ = No dirrect modification!
}

Good

componentDidMount() {
  this.setState(prevState => ({
    emoji: [...prevState.emoji, {}]
  }))
}

Basket functionallity doesn't work

The usage of dangerouslySetInnerHTML attribute is redundant

There is no package.json file in the project structure. It is not possible to run the project

Do not store "render depending code" in class instance variables -- use this.state = {}.

Bad

class Foo extends React.Component {
  constructor(props) {
    super(props);

    this.counter = 0;
  }

  componentDidMount() {
    this.counter += 1; // Value will be changed, but component will not be updated!
  }

  render() {
    return (
      <p>{this.counter}</p> // 0
    )
  }
}

Good

class Foo extends React.Component {
  constructor(props) {
    super(props);

    this.counter = 0;
  }

  componentDidMount() {
    this.setState(prevState => ({
      counter: prevState.counter + 1, // Good. React tracks the `this.state` variable and if something changes component will be updated!
    });
  }

  render() {
    return (
      <p>{this.counter}</p> // 1
    )
  }
}

Data fetching should be implemented on the component level

Bad

fetch('http://example.com/api/data')
  .then(response => response.json())
  .then(data => {
    ReactDOM.render(<App data={data} />, document.querySelector('#root')))
  })

Good

class Foo extends React.Component {

  constructor(props) {
    super(props);

    this.state = {
      data: [],
      isLoading: false,
    }
  }

  componentDidMount() {
    this.setState({ isLoading: true });
    fetch('http://example.com/api/data')
      .then(response => response.json())
      .then(data => {
        this.setState({ data: data, isLoading: false });
      });
  }

  render() {
    if (this.state.isLoading) {
      return <p>Loading...</p>;
    }

    return (
      // Code goes here
    )
  }
}

General

Emoji icons are "hardcoded" in the component (copy pasted from response and inserted manually in component). You had to get the icons from /emoji-shop response and pass to the component (programattically)

Emoji rating is rendered incorrectly. The rating emoji rating 4.5 rendered as 4 full stars and empty. Should be 3 full, 1 half and 1 empty

Emoji promotion section is "hardcoded" (top banner)

Poor file organization within project

Poor code organization within a component (hard to read)

Warnings and errors in the browser console (e.g. "Warning: Each child in a list should have a unique "key" prop")

Advices

Pass entire object to the component props

Not perfect

<Component a={foo.a} b={foo.b} c={foo.c} d={foo.d} />

Better

<Component foo={foo} />

Remove "dead code" (a.k.a. commented code, unsuded code) from the commits

Use PropTypes as much as possible (advanced TypeScript, Flow). Although it is much longer, it will help you in the future.

Don't use React.Fragment (e.g. <></>) around single element

Not perfect

function Component(props) {
  return (
    <> // <- Redundant
      <p>Hello</p>
    </> // <- Redundant
  )
}

Perfect

function Component(props) {
  return <p>Hello</p>;
}

Not all parameters are described in PropTypes. Keep in sync everytime you make changes

Do not split components into smaller and smaller and smaller and smaller components. If some logic and visualization can be reused in a different place then create a component. (P.S. Understanding will come by itself)

Bad practice to use global styles in React. Use CSS modules (advanced styled-components)

Use Functional component over Class component if there are no lifecycle hooks or state inside

Move complex logic from iteration function into a separate method. It will bring additional understanding of what will be rendred to the screen, even before reading the code implementation.

Not perfect

function Component(props) {
  return props.emoji.map((item, index) => {
    return (
      <div>
        <div>
          <span>Emoji name</span>
          <span>Emoji rating</span>
          <span>Emoji Code</span>
        </div>
        <button>Get $1.5</button>
      <div>
    )
  })
}

Perfect

function Component(props) {
  function renderEmoji(item, index) {
    return (
      <div>
        <div>
          <span>Emoji name</span>
          <span>Emoji rating</span>
          <span>Emoji Code</span>
        </div>
        <button>Get $1.5</button>
      <div>
    )
  }

  return props.emoji.map(renderEmoji)
}

Thank you a lot (: