TransWikia.com

Todo application written in react

Code Review Asked by mister nobody on December 28, 2021

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 🙂

react.js

One Answer

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>

Answered by Drew Reese on December 28, 2021

Add your own answers!

Ask a Question

Get help from others!

Recent Answers

Recent Questions

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP