React programming pattern

Question

I create https://github.com/storenth/react-redux-todo and ask to review my Todo-react-app for programming pattern: dividing components into presentational components and container components.

Here’s the basic idea behind it: if a component has to have state, make calculations based on props, or manage any other complex logic, then that component shouldn’t also have to render HTML-like JSX.

Instead of rendering HTML-like JSX, the component should render another component. It should be that component’s job to render HTML-like JSX.

So, my question about the /containers/TodosContainer that contains /components/Todos and /components/AddForm. Does it match the pattern? I didn't get where to place the AddForm, inside /components/Todos (to complex I guess)?

import React, { Component, Fragment } from 'react';
import { Todos } from "../components/Todos";
import { AddForm } from "../components/AddForm";

export class TodosContainer extends Component {
    constructor(props) {
        super(props);
        this.state = { items: [], text: '' };
        this.handleChange = this.handleChange.bind(this);
        this.handleSubmit = this.handleSubmit.bind(this);
        this.removeItem = this.removeItem.bind(this);
    }
    handleChange = (e) => {
        this.setState({
            text: e.target.value
        })
    }
    handleSubmit = (e) => {
        e.preventDefault();
        if (this.state.text.length === 0) {
            return;
        }
        const newItem = {
            text: this.state.text,
            id: Date.now()
        };
        this.setState({
            items: this.state.items.concat(newItem),
            text: ''
        })
    }
    removeItem = (id) => {
        const items = this.state.items.filter(item => {
            return item.id !== id;
        })
        this.setState({
            items
        })
        console.log(this.state);
    }

    render() {
        return (
            <>
                <Todos items={this.state.items} removeItem={this.removeItem} />
                <AddForm items={this.state} onSubmit={this.handleSubmit} onChange={this.handleChange} />
            </>
        )
    }
}
```

Answer 1

I'd say, if it's the Presentation and Container Components pattern as described by Dan Abramov, then yes, you've pretty effectively nailed it. (Though Dan no longer advocates this pattern!!)

Suggestions/Review

AddForm

items prop is really the text state in the parent component, it doesn't need the entire state object. (If it does then name it more accurately!) What's used is just the input value, so value is a better descriptor.

export const AddForm = ({ value, onSubmit, onChange }) => {
  console.log(value);
  return (
    <div className="add-item-form">
      <form onSubmit={e => onSubmit(e)}>
        <label htmlFor="name">Add new todo:</label>
        <input
          type="text"
          id="name"
          onChange={e => onChange(e)}
          value={value}
        />
        <button
          className="btn waves-effect waves-light"
          type="submit"
          name="action"
        >
          Add item
        </button>
      </form>
    </div>
  );
};

Usage: <AddForm value={this.state.text} ... />

TodosContainer

The handlers are all arrow functions, they already have the this of the class bound to them, so there isn't need to bind them in a constructor. In fact, if all the constructor is doing is setting the initial state, then you don't really need the constructor either as you can simply set initial state with a state instance variable.

Instead of

constructor(props) {
  super(props);
  this.state = { items: [], text: '' };
  this.handleChange = this.handleChange.bind(this);
  this.handleSubmit = this.handleSubmit.bind(this);
  this.removeItem = this.removeItem.bind(this);
}

Simply

state = { items: [], text: '' };

State updates

When any next state depends upon current state, like incrementing counters, or adding values to arrays, you should get into the habit of using functional state updates, and also not referencing this.state within the state update.

State and Lifecycle setState

handleSubmit can be improved

handleSubmit = (e) => {
  e.preventDefault();
  const newItem = {
    text: this.state.text,
    id: Date.now(),
  };
  if (this.state.text) { // <-- if text is truthy, i.e. non-empty string
    this.setState(prevState => ({
      items: prevState.items.concat(newItem), 
    }));
  }
}

State updates are also asynchronous, so don't try to console.log it after requesting the state update. Use a component lifecycle like componentDidUpdate to log state after the update (or for class-based components, you can use the setState 2nd parameter callback function, but for simplicity sake stick to the lifecycle functions.).

removeItem handler can be improved

removeItem = (id) => {
  this.setState(prevState => ({
    items: prevState.items.filter(item => item.id !== id),
  }));
}