React Todo with Sass

Community feedback

• 

  Kostiantin•80

  @jebbaitedPosted 9 months ago

  Hi!

  1: Do not use create-react-app. Use Vite instead. 2: Follow DRY (Don't repeat yourself) principle. You use the same array as initial state twice. Create it outside of component and then pass to useState().

  const [allTodos, setAllTodos] = useState([
      {
        description: "Jog in the park",
        isCompleted: false,
        id: 1,
      },
      {
        description: "Walk the dog",
        isCompleted: false,
        id: 2,
      },
      {
        description: "Walk the dog",
        isCompleted: false,
        id: 3,
      },
    ]);
  

  Same here

   setAllTodos([
        ...allTodos,
        {
          description: `${value}`,
          isCompleted: false,
          id: Date.now(),
        },
      ])
      setFilteredTodos([
        ...allTodos,
        {
          description: `${value}`,
          isCompleted: false,
          id: Date.now(),
        },
      ])
  

  3: Why do you pass params to function and don't use them? Use can replcae 'e' with an underscore '_' as a placeholder for function parameter that is not used.

    const dragStart = (e, index) => {
      dragItem.current = index;
    };
  

    const dragStart = (_, index) => {
      dragOverItem.current = index;
    };
  

  But in this case it is better to not pass 'e' at all. Remove this param. In several component I found such functions. Example:

  onDragStart={(e) => dragStart(e, index)}
  onDragEnter={(e) => dragEnter(e, index)}
  

  4: For many conditions in classnames I'd use library CLSX or classnames. Because some statements are unreadable. Like this one:

      className={`${
                currentTodo.isCompleted === true ? "todo-item--completed" : null
              } todo-item ${
                currentTheme === "light" ? "todo-item--light" : "todo-item--dark"
              } ${
                currentTodo.isCompleted && currentTheme === "light"
                  ? "todo-item--completed--light"
                  : ""
              } 
                
                ${
                  currentTodo.isCompleted && currentTheme === "dark"
                    ? "todo-item--completed--dark"
                    : ""
                } round`}
  

  5: Unnecessary ref.

  const searchInput = useRef(null)
  

  6: If you need a unique id just use React hook useId() DOCS instead of Date():

    {
          description: `${value}`,
          isCompleted: false,
          id: Date.now(),
    },
  

  7: For theme toggling use useContext() hook. There are a lot of example you can find. One of them is Medium. 8: Avoid reapiting elements. Check out my previous answers for other project and you'll find out how to fix it.

      <button className="button--footer" onClick={showAll}>
              All
            </button>
            <button className="button--footer" onClick={showActive}>
              Active
            </button>
            <button className="button--footer" onClick={showCompleted}>
              Completed
            </button>
  

  9: Bad practice to attach events to wrong elements. Yes, it would work. But your <label /> isn't a button. Why do you handle click on it?

     <input type="checkbox" className="checkbox" />
        <label
          onClick={handleClick}
          htmlFor="checkbox"
          className={`checkbox-label ${
            currentTheme === "light"
              ? "checkbox-label--light"
              : "checkbox-label--dark"
          } ${isCompleted ? "label-checked" : ""}`}
        >
  

  Change your code to this and it would be better:

  <input type="checkbox" id={`checkbox-${id}`} className="checkbox" onClick={handleClick} />
        <label
          htmlFor={`checkbox-${id}`}
          className={`checkbox-label ${
            currentTheme === 'light' ? 'checkbox-label--light' : 'checkbox-label--dark'
          } ${isCompleted ? 'label-checked' : ''}`}
        >
  

  To handle click on label you need the same id on label and input. 10: Your <Todo /> doesn't need to know about all todos. So, we can change it. Remove some props and move handlers to parent component.

    <Todo
                todo={currentTodo}
                currentTheme={currentTheme}
                handleDelete={handleDelete}
                handleClick={handleClick}
      />
  

  TodoList.js

    const handleDelete = (id) => {
      const filtered = allTodos.filter((todo) => todo.id !== id)
  
      setAllTodos(filtered)
      setFilteredTodos(filtered)
    }
  
    const handleClick = (id) => {
      const updatedArray = allTodos.map((currentTodo) => {
        if (currentTodo.id === id) {
          return {
            ...currentTodo,
            isCompleted: !currentTodo.isCompleted,
          }
        }
  
        return currentTodo
      })
  
      setAllTodos(updatedArray)
      setFilteredTodos(updatedArray)
    }
  

  Now <ToDo /> is:

  export default function Todo({ todo, currentTheme, handleDelete, handleClick }) {
    return (
      <>
        <input
          type="checkbox"
          id={`checkbox-${todo.id}`}
          className="checkbox"
          onClick={() => handleClick(todo.id)}
        />
        <label
          htmlFor={`checkbox-${todo.id}`}
          className={`checkbox-label ${
            currentTheme === 'light' ? 'checkbox-label--light' : 'checkbox-label--dark'
          } ${todo.isCompleted ? 'label-checked' : ''}`}
        >
          <svg className="check" xmlns="http://www.w3.org/2000/svg" width="11" height="9">
            <path
              fill="none"
              className={`${currentTheme === 'light' ? 'check--light' : 'check--dark'} ${
                todo.isCompleted ? 'label-checked' : ''
              }`}
              strokeWidth="2"
              d="M1 4.304L3.696 7l6-6"
            />
          </svg>
        </label>
        <span>{todo.description}</span>
        <svg
          className="cross"
          onClick={() => handleDelete(todo.id)}
          xmlns="http://www.w3.org/2000/svg"
          width="18"
          height="18"
        >
          <path
            fill="#494C6B"
            fillRule="evenodd"
            d="M16.97 0l.708.707L9.546 8.84l8.132 8.132-.707.707-8.132-8.132-8.132 8.132L0 16.97l8.132-8.132L0 .707.707 0 8.84 8.132 16.971 0z"
          />
        </svg>
      </>
    )
  }
  

  Figure this out. If have questions - feel free to ask.

  Marked as helpful

  0

  ---

  

  P

  Zsofia Dalnoki-Papp•210

  @dalnokiPosted 8 months ago

  @jebbaited

  Hi there, thanks a lot for reviewing my project in detail!

  1: I changed the project to Vite

  2: got rid of the duplicated array, thanks

  3: if I completely remove e, the function breaks so I changed it to underscore

  4: Thanks for the suggestion, clsx is awesome!

  5: Removed the unnecessary ref

  6: I decided to go with uuid instead but thanks for the suggestion

  7: Thanks, now the theme change is powered by useContext

  8: Thanks, took care of it

  9: If you check the CSS, the actual input is hidden and the label is styled and should handle the click

  10: Yes, true, fixed it.

  0