REACT + VITE TO DO APP

Community feedback

• 

  Pavel B.•270

  @JagholinPosted over 1 year ago

  Some of the things that I would do differently:

  • you don't really need Sun and Moon components. Since you are using vitejs, it supports importing image assets and you can just set the svg files either as src attribute for an <img> tag, or as background-image CSS property on a div.
  • I find it better to destructure props immediately in the function argument list. Like this:

  const Sun = ({onClick}) => {...}
  

  This way, it is immediately apparent what props a component expects to receive, and you dont need to read the rest of the function code to find it out. But this is just my preference :)

  • when you have dependent variables like incompleteCount in the ToDo component, you can use useMemo hook instead, and get rid of associated useEffect. Like so:

  const incompleteCount = useMemo(
      () => toDoList.filter(item => !item.complete).length, 
      [toDoList]);
  

  • newList.splice(index, 0, newList.splice(newList.indexOf(draggedItem), 1)[0]) is a bit hard to read. I would write it out:

  newList.splice(newList.indexOf(draggedItem), 1); 
  newList.splice(index, 0, draggedItem);
  

  • I wouldn't use *.classList.add / remove functions, because they do the same thing as className JSX attribute. When you have multiple ways of doing the same thing, prefer to use that which is more idiomatic to the libraries you use.

  • This is a terrible way of generating "unique" ids: Math.floor(Math.random() * 5000000). Just use natural numbers in order: 0, 1, 2, 3 etc.

  • About your original way of dealing with mobile/desktop layouts... I don't see the point in doing it like this. Not only are you duplicating your code, but your site actually breaks if you resize it (ToDo component is unmounted and all data is lost)

  PS and additional thing:

  • It is good that you use key prop where React requires you to, but it seems that you dont really understand what it supposed to do. key allows React to distinguish components in an array, giving them something to be identified by. index here is not a good key, since it is possible to rearrange todo elements. You already have task.id property, so use it instead.

  <div 
                  key={entry.id}
                  ...
  >
  

  Marked as helpful

  1