Todo application written in react

Question

Aware that this is a very common exercise; but I'm getting a bit obsessed about creating a simple, clean-ui todo app, at least for myself.

It's not quite finished yet, but I guess it will be much easier to review now than at the end, in case you look at the javascript code (react)

I'm looking any simple advice, best practice or error in the javascript-react file, or in the css html too.

function H1(){
 let hello = "Hi,"
 let h1 = (
 <h1>
 {hello}
 </h1> 
 )
 return h1 
}
class Todos extends React.Component{
 constructor(){
 super()
 this.state={todos:[]}
 //todos will be an array of objects
 this.addTodo = this.addTodo.bind(this)
 this.toggleCompleted = this.toggleCompleted.bind(this)
 }
 addTodo(){
 //press go -> take input value
 let theValue = document.getElementById('input').value
 //push array w value and id
 this.setState(s => s.todos.push({id:s.todos.length, value:theValue, completed:false}))
 return 1 
 }
 toggleCompleted(e){
 //toggle completed value in the state object
 let parentId = e.target.parentElement.id
 let completed = this.state.todos[parentId].completed
 if (completed){ 
 this.setState(s => s.todos[parentId].completed=false)
 }
 else {
 this.setState(s => s.todos[parentId].completed=true)
 }
 }
 render() {
 let listOfTodos = ""
 if (this.state.todos !== []){
 listOfTodos = this.state.todos.map(todo => <TodoItem toggler={this.toggleCompleted} key={todo.id} thisItem={todo}/>)
 }
 return (
 <div className="todos__area">
 <div className="todos__events">
 <input id='input' className="todos__input" placeholder="type here" type="text" />
 <button onClick = {this.addTodo} className="todos__button">Go</button>
 </div>
 <ul className="todos__list">
 {listOfTodos}
 </ul> 
 </div>
 )
 }
}
function TodoItem(props){
 return (<li id={props.thisItem.id}>
 <input onClick={props.toggler} className="checkbox" type='checkbox'/>
 <span style=
 {{textDecoration:props.thisItem.completed?"line-through":"none"}}>
 {props.thisItem.value}
 </span> </li>) 
}
function Footer(){
return (
 <footer>
 <ul className="footer__ul pointer">
 <li>Github</li>
 <li>Reddit</li>
 <li>FCC</li>
 </ul> 
 </footer>
)
}
function App(){
 //render components
 return (
 <main>
 <H1 />
 <Todos />
 <Footer />
 </main>
 )
}
ReactDOM.render(<App/>, document.getElementById('root'))

* {
 padding: 0;
 margin: 0;
 box-sizing: border-box;
}
:root {
 --blue:#2c3251;
}
ul {
 list-style-type: none;
}
ul > li {
 display: block;
 padding: 0.5rem;
}
.pointer > li:hover {
 cursor: pointer;
}
body {
 font-family: "Ubuntu", sans-serif;
 text-align: left;
 background-color: white;
 background-image: radial-gradient(rgba(0, 0, 0, 0.1), rgba(0, 0, 0, 0.6)), url("./table.png");
 background-repeat: repeat;
 padding: 2rem 2rem 0.1rem;
}
main {
 display: grid;
 min-height: auto;
 height: 100vh;
 width: 100%;
}
h1 {
 font-size: 1.8rem;
 color: var(--blue);
 box-shadow: inset 0px 2px 5px var(--blue);
 align-self: start;
 padding: 1.5rem;
 border-radius: 15px 15px 0 0;
 margin-bottom: 0.5rem;
}
.todos__area {
 box-shadow: 1px 1px 1px rgba(0, 0, 0, 0.7), 1px 1px 5px rgba(0, 0, 0, 0.3);
 display: flex;
 flex-direction: column;
 max-height: 300px;
}
.todos__events {
 display: flex;
 flex-direction: row;
 flex: 0 1 40px;
}
.todos__events .todos__input {
 border: none;
 padding: 0.5rem;
 border-radius: 2px 0 0 2px;
 flex: 2 1 150px;
}
.todos__events .todos__button {
 border: none;
 padding: 10px;
 flex: 1 1 60px;
 color: white;
 font-weight: bold;
 cursor: pointer;
 max-width: 100px;
 position: relative;
 background-color: rgba(0, 0, 0, 0);
}
.todos__events .todos__button:after {
 z-index: -1;
 position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 height: 100%;
 background-color: #2d7b57;
 opacity: 0.5;
 transition: opacity 0.3s ease-out;
 content: "";
}
.todos__events .todos__button:hover:after {
 opacity: 0.7;
}
/* Inline #1 | http://localhost:3000/ */
.todos__list {
 font-family: "Dancing Script", "cursive";
 font-size: 1.2rem;
 padding: 2rem;
 background-color: rgba(227, 227, 134, 0.58);
 flex: 1 1 200px;
 overflow: auto;
}
.todos__list input[type=checkbox] {
 margin-right: 0.6rem;
}
footer {
 background-color: rgba(0, 0, 0, 0.1);
 align-self: end;
 margin-top: 0.5rem;
}
.footer__ul {
 display: flex;
 justify-content: center;
 color: var(--blue);
 font-weight: bold;
}
.footer__ul > li {
 padding: 1rem;
 box-shadow: 1px 0px 1px var(--blue);
 transition: transform 0.1s ease-out;
}
.footer__ul > li:hover {
 transform: scaleX(1.05);
}
@media screen and (min-width: 700px) {
 body {
 max-width: 700px;
 margin: auto;
 }
}
/*# sourceMappingURL=index.css.map */

<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.6.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.6.3/umd/react-dom.production.min.js"></script>
<!DOCTYPE html>
<html lang="en">
 <link href="https://fonts.googleapis.com/css2?family=Dancing+Script&family=Ubuntu&display=swap" rel="stylesheet">
 <head>
 <meta charset="utf-8" />
 <title>React App: todoist</title>
 </head>
 <body >
<div id="root"></div>
 </body>
</html>

A few things yet to add:

• "delete button"
• "done" button (and probably remove the checkbox)
• color scheme and welcome message ("Hi") changing with the time of the day.
• the "go" button, should work by hitting enter/return too. I'm not sure how to do that so far.

Hope you like :-)

Answer 1

You've several issues with state mutation, which is a major anti-pattern in react, poor variable declarations, and other sub-optimal coding style.

constructor

1. The constructor is missing props (and passing them to super). It doesn't appear that currently any props are passed to Todos, but should this ever change it could cause issue later on down the road. Better to just assume some props can be passed.

updates

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

addToDo

addTodo(){
 //press go -> take input value
 let theValue = document.getElementById('input').value;
 //push array w value and id
 this.setState(s => s.todos.push({id:s.todos.length, value: theValue, completed:false}));
 return 1;
}

1. theValue never changes so it should be declared const.
2. Pushing into an array mutates the existing array and returns the new length, so not only did this mutate existing state, it updated it to no longer be an array. Use a correct functional state update that shallowly copies previous state.
3. addToDo is used as an onClick handler, so the return is meaningless.
4. document.getElementById is generally also considered an anti-pattern. Using a ref is the more "react way" of getting the value

User Experience (UX): After finishing a code review I ran your code snippet and noticed the input doesn't clear after being added. Just a suggestion here, but maybe clear out the input field upon adding a todo item.

updates

constructor() {
 super();
 ...
 this.inputRef = React.createRef(); // <-- create input ref
 ...
}
addTodo(){
 // press go -> take input value
 const { value } = this.inputRef.current;
 // shallow copy existing state and append new value
 this.setState(({ todos }) => ({
 todos: [...todos, { id: todos.length, value, completed: false }],
 }));
 // Suggestion to clear input
 this.inputRef.current.value = '';
}
...
<input
 ref={this.inputRef} // <-- attach inputRef to input
 id='input'
 className="todos__input"
 placeholder="type here"
 type="text"
/>
...

toggleCompleted

toggleCompleted(e){
 //toggle completed value in the state object
 let parentId = e.target.parentElement.id
 let completed = this.state.todos[parentId].completed
 if (completed){ 
 this.setState(s => s.todos[parentId].completed=false)
 } else {
 this.setState(s => s.todos[parentId].completed=true)
 }
}

1. Variables parentId and completed don't change, so should also be declared const.
2. Similar issue with state mutation. You still need to shallowly copy the existing state and update the element by index/id.
3. The two logic branches of if (completed) are nearly identical, a more DRY approach would be to do the branching at the value, i.e. using a ternary, or even better, just simply toggle the boolean value, like the function's name suggests.
4. Get the id of the target element of the event object (more on this later)

updates

toggleCompleted(e){
 // toggle completed value in the state object
 const { id } = e.target;
 this.setState(({ todos }) => ({
 todos: todos.map(todo => todo.id === Number(id) ? { // <-- id is string
 ...todo,
 completed: !todo.completed,
 } : todo),
 }));
}

render

1. So long as this.state.todos is an array, the map function can correctly handle an empty array, no need really to test that it isn't equal to an empty array ([]), but if there is concern it is more common to conditionally render with a check on the length, i.e. this.state.todos.length && this.state.todos.map(....

updates

render() {
 return (
 <div className="todos__area">
 <div className="todos__events">
 <input ref={this.inputRef} id='input' className="todos__input" placeholder="type here" type="text" />
 <button onClick={this.addTodo} className="todos__button">Go</button>
 </div>
 <ul className="todos__list">
 {this.state.todos.map(todo => (
 <TodoItem
 key={todo.id}
 toggler={this.toggleCompleted}
 thisItem={todo}
 />
 ))}
 </ul> 
 </div>
 )
}

TodoItem

1. The input checkbox should probably use the onChange handler versus an onClick, it's semantically more correct.
2. Attach the id to the input instead of the parent node.
3. Set the checked value of the input to the item completed status.
4. Wrap the input and span in a label so it can be clicked on to toggle the completed state.

updates

function TodoItem({ thisItem, toggler }){
 return (
 <li>
 <label>
 <input
 id={thisItem.id}
 checked={thisItem.completed}
 className="checkbox"
 onChange={toggler}
 type='checkbox'
 />
 <span
 style={{
 textDecoration: thisItem.completed ? "line-through" : "none"
 }}
 >
 {thisItem.value}
 </span>
 </label>
 </li>
 ) 
}

Running Demo

function H1() {
 let hello = "Hi,";
 let h1 = <h1> {hello} </h1>;
 return h1;
}
class Todos extends React.Component {
 constructor() {
 super();
 this.state = {
 todos: []
 };
 this.inputRef = React.createRef(); // <-- create input ref
 //todos will be an array of objects
 this.addTodo = this.addTodo.bind(this);
 this.toggleCompleted = this.toggleCompleted.bind(this);
 }
 addTodo() {
 // press go -> take input value
 const { value } = this.inputRef.current;
 // shallow copy existing state and append new value
 this.setState(({ todos }) => ({
 todos: [
 ...todos,
 {
 id: todos.length,
 value,
 completed: false
 }
 ]
 }));
 // Suggestion to clear input
 this.inputRef.current.value = "";
 }
 toggleCompleted(e) {
 // toggle completed value in the state object
 const { id } = e.target;
 this.setState(({ todos }) => ({
 todos: todos.map(todo =>
 todo.id === Number(id)
 ? {
 ...todo,
 completed: !todo.completed
 }
 : todo
 )
 }));
 }
 render() {
 return (
 <div className="todos__area">
 <div className="todos__events">
 <input
 ref={this.inputRef}
 id="input"
 className="todos__input"
 placeholder="type here"
 type="text"
 />
 <button onClick={this.addTodo} className="todos__button">
 {" "}
 Go{" "}
 </button>{" "}
 </div>{" "}
 <ul className="todos__list">
 {" "}
 {this.state.todos.map(todo => (
 <TodoItem
 key={todo.id}
 toggler={this.toggleCompleted}
 thisItem={todo}
 />
 ))}{" "}
 </ul>{" "}
 </div>
 );
 }
}
function TodoItem({ thisItem, toggler }) {
 return (
 <li>
 <label>
 <input
 id={thisItem.id}
 checked={thisItem.completed}
 className="checkbox"
 onChange={toggler}
 type="checkbox"
 />
 <span
 style={{
 textDecoration: thisItem.completed ? "line-through" : "none"
 }}
 >
 {thisItem.value}{" "}
 </span>{" "}
 </label>{" "}
 </li>
 );
}
function Footer() {
 return (
 <footer>
 <ul className="footer__ul pointer">
 <li> Github </li> <li> Reddit </li> <li> FCC </li>{" "}
 </ul>{" "}
 </footer>
 );
}
function App() {
 //render components
 return (
 <main>
 <H1 />
 <Todos />
 <Footer />
 </main>
 );
}
ReactDOM.render( < App / > , document.getElementById('root'))

* {
 padding: 0;
 margin: 0;
 box-sizing: border-box;
}
:root {
 --blue: #2c3251;
}
ul {
 list-style-type: none;
}
ul>li {
 display: block;
 padding: 0.5rem;
}
.pointer>li:hover {
 cursor: pointer;
}
body {
 font-family: "Ubuntu", sans-serif;
 text-align: left;
 background-color: white;
 background-image: radial-gradient(rgba(0, 0, 0, 0.1), rgba(0, 0, 0, 0.6)), url("./table.png");
 background-repeat: repeat;
 padding: 2rem 2rem 0.1rem;
}
main {
 display: grid;
 min-height: auto;
 height: 100vh;
 width: 100%;
}
h1 {
 font-size: 1.8rem;
 color: var(--blue);
 box-shadow: inset 0px 2px 5px var(--blue);
 align-self: start;
 padding: 1.5rem;
 border-radius: 15px 15px 0 0;
 margin-bottom: 0.5rem;
}
.todos__area {
 box-shadow: 1px 1px 1px rgba(0, 0, 0, 0.7), 1px 1px 5px rgba(0, 0, 0, 0.3);
 display: flex;
 flex-direction: column;
 max-height: 300px;
}
.todos__events {
 display: flex;
 flex-direction: row;
 flex: 0 1 40px;
}
.todos__events .todos__input {
 border: none;
 padding: 0.5rem;
 border-radius: 2px 0 0 2px;
 flex: 2 1 150px;
}
.todos__events .todos__button {
 border: none;
 padding: 10px;
 flex: 1 1 60px;
 color: white;
 font-weight: bold;
 cursor: pointer;
 max-width: 100px;
 position: relative;
 background-color: rgba(0, 0, 0, 0);
}
.todos__events .todos__button:after {
 z-index: -1;
 position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 height: 100%;
 background-color: #2d7b57;
 opacity: 0.5;
 transition: opacity 0.3s ease-out;
 content: "";
}
.todos__events .todos__button:hover:after {
 opacity: 0.7;
}
/* Inline #1 | http://localhost:3000/ */
.todos__list {
 font-family: "Dancing Script", "cursive";
 font-size: 1.2rem;
 padding: 2rem;
 background-color: rgba(227, 227, 134, 0.58);
 flex: 1 1 200px;
 overflow: auto;
}
.todos__list input[type=checkbox] {
 margin-right: 0.6rem;
}
footer {
 background-color: rgba(0, 0, 0, 0.1);
 align-self: end;
 margin-top: 0.5rem;
}
.footer__ul {
 display: flex;
 justify-content: center;
 color: var(--blue);
 font-weight: bold;
}
.footer__ul>li {
 padding: 1rem;
 box-shadow: 1px 0px 1px var(--blue);
 transition: transform 0.1s ease-out;
}
.footer__ul>li:hover {
 transform: scaleX(1.05);
}
@media screen and (min-width: 700px) {
 body {
 max-width: 700px;
 margin: auto;
 }
}
/*# sourceMappingURL=index.css.map */

<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.8.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.8.3/umd/react-dom.production.min.js"></script>
<!DOCTYPE html>
<html lang="en">
<link href="https://fonts.googleapis.com/css2?family=Dancing+Script&family=Ubuntu&display=swap" rel="stylesheet">
<head>
 <meta charset="utf-8" />
 <title>React App: todoist</title>
</head>
<body>
 <div id="root"></div>
</body>
</html>