React Todo List App

Question

I have made a small app in React that fulfils the following user stories:

GIVEN I add an item using the form 
THEN I see the text appear in the list below

GIVEN I see items in the list
AND I click the close button next to a list item
THEN the list item disappears from the page

GIVEN I refresh the page at any point
THEN I see the list that I saw on the page before I refreshed

Here is my code:

const TodoInput = props => {
    return (
        <form>
            <div>Enter a todo</div>
            <input
                type="text"
                onChange={props.onHandleChangeInputVal}
                value={props.inputVal}
            />
            <button
                type="submit"
                onClick={props.onHandleAddTodo}
            >
                Add to the list
            </button>
        </form>
    )
};

class TodosList extends React.Component {

    constructor() {
        super();
        this.state = {
            inputVal: '',
            todos: []
        };

        this.onHandleChangeInputVal = this.onHandleChangeInputVal.bind(this);
        this.onHandleAddTodo = this.onHandleAddTodo.bind(this);
    }

    componentDidMount() {
        const todosFromLocalStorage = JSON.parse(localStorage.getItem('localStorageTodos'));

        this.setState({
            todos: todosFromLocalStorage || []
        })
    }

    getTodos() {
        const todos = this.state.todos;

        return todos.map((todo, index) => {
            return (
                <li
                    key={index}
                >
                    {todo}
                    <button onClick={event => this.onHandleDeleteTodo(event, index)}>&times;</button>
                </li>
            )
        })
    }

    onHandleChangeInputVal(e) {
        const newInputVal = e.target.value;

        this.setState({
            inputVal: newInputVal
        })
    }

    onHandleAddTodo(e) {
        e.preventDefault();
        let todosCopy = this.state.todos.slice();

        if (this.state.inputVal) {
            todosCopy.push(this.state.inputVal);
        }

        this.setState({
            todos: todosCopy,
            inputVal: ''
        }, () => {
            this.updateLocalStorageWithState();
        });

    }

    onHandleDeleteTodo(event, indexOfItemToRemove) {
        event.preventDefault();

        let todosCopy = this.state.todos.slice();

        todosCopy.splice(indexOfItemToRemove, 1);

        this.setState({
            todos: todosCopy
        }, () => {
            this.updateLocalStorageWithState();
        });

    }

    updateLocalStorageWithState() {
        localStorage.setItem('localStorageTodos', JSON.stringify(this.state.todos));
    }

    render() {
        const todos = this.getTodos();

        return (
            <div>
                <TodoInput
                    onHandleChangeInputVal={this.onHandleChangeInputVal}
                    onHandleAddTodo={this.onHandleAddTodo}
                    inputVal={this.state.inputVal}
                />
                <ul>{todos}</ul>
            </div>
        )
    }
}

ReactDOM.render(<TodosList />, document.getElementById('app'));

Here is a demo: http://codepen.io/alanbuchanan/pen/RoKBLr?editors=0010

Please can you review for any improvements to how I have structured and written my code?

inyono · Accepted Answer · 2016-11-20 03:30:42Z

Overall, I like your code. Especially, since you have the TodoInput as a separate stateless component. IMHO, there is still some room for improvement, though:

You shouldn't use setState in componentDidMount since this will trigger a re-rendering. Since localStorage.getItem is synchronous anyway, you can initialise the state in the constructor and remove componentDidMount completely.
If you change the signature of onHandleDeleteTodo, you don't need to reference event in getTodos:

getTodos() {
  // ...
  <button onClick={this.onHandleDeleteTodo(index)}
  // ...
}

onHandleDeleteTodo(indexOfItemToRemove) {
  return (event) => {
    event.preventDefault();
    let todosCopy = this.state.todos.slice();

    todosCopy.splice(indexOfItemToRemove, 1);

    this.setState({
        todos: todosCopy
    }, () => {
        this.updateLocalStorageWithState();
    });
  }
}

I would probably use Array.concat instead of Array.slice and Array.push (in onHandleAddTodo)
You don't need the extra arrow function in onHandleAddTodo:

this.setState({
  todos: todosCopy
}, () => {
  this.updateLocalStorageWithState();
});

is equivalent to

this.setState({
  todos: todosCopy
}, this.updateLocalStorageWithState);

The prefix onHandle doesn't sound right to me. I would probably just use on for the naming of the prop (i.e. props.onChangeInputVal in TodoInput) and handle for the actual implementation that will be passed down (i.e. handleChangeInputVal in TodosList. Furthermore, I would rename the prop inputVal to value since that is the naming convention used for form elements in React.

Stack Exchange Network

React Todo List App

1 Answer 1

You must log in to answer this question.

Hot Network Questions

React Todo List App

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions