Created Todo app #679

davinnchii · 2023-09-12T10:39:10Z

BudnikOleksii

Good work 👍
Left some comments to improve your solution.
Use functional setState and array iteration methods to update your todos

BudnikOleksii · 2023-09-15T16:00:42Z

src/TodosContext.tsx

+// [JSON.parse(localStorage.getItem('todos')
+//   || '{}')] as Todo[],


Remove comments

BudnikOleksii · 2023-09-15T16:03:23Z

src/components/TodoApp/TodoApp.tsx

+type Props = {};
+
+export const TodoApp: React.FC<Props> = () => {
+  const { setTodos } = useContext(TodosContext);


Create a hook for your context instead of passing TodosContext every time

const useTodos = useContext(TodosContext);

BudnikOleksii · 2023-09-15T16:04:33Z

src/components/TodoApp/TodoApp.tsx

+      setTodos((prev: Todo[]) => [...prev,
+        { id: uuidv4(), title: newTodoTitle, completed: false }]);


Suggested change

setTodos((prev: Todo[]) => [...prev,

{ id: uuidv4(), title: newTodoTitle, completed: false }]);

setTodos((prev: Todo[]) => [

...prev,

{ id: uuidv4(), title: newTodoTitle, completed: false }

]);

BudnikOleksii · 2023-09-15T16:07:53Z

src/components/TodoItem/TodoItem.tsx

+  const index = todos.findIndex(({ id }) => id === todo.id);
+
+  const handleRemoveTodo = () => {
+    const todosCopy = [...todos];
+
+    todosCopy.splice(index, 1);
+
+    setTodos(todosCopy);
+  };


Don't use splice in this case, use filter

setTodos(previousTodos => previousTodos.filter( currentTodo => todo.id !== currentTodo.id ));

BudnikOleksii · 2023-09-15T16:08:57Z

src/components/TodoItem/TodoItem.tsx

+  const handleCompleteTodo = (
+    event: React.ChangeEvent<HTMLInputElement>,
+  ) => {
+    const todosCopy = [...todos];
+
+    todosCopy.splice(index, 1, { ...todo, completed: event.target.checked });
+
+    setTodos(todosCopy);
+  };


Use map instead of splice to change your todo

BudnikOleksii · 2023-09-15T16:09:27Z

src/components/TodoItem/TodoItem.tsx

+  const handleEditTodo = (
+    title: string,
+  ) => {
+    const todosCopy = [...todos];
+
+    if (title && title !== todo.title) {
+      todosCopy.splice(index, 1, { ...todo, title });
+
+      setTodos(todosCopy);
+      setIsEditingEnabled(false);
+    }


The same, don't use splice

BudnikOleksii · 2023-09-15T16:12:39Z

src/components/TodoList/TodoList.tsx

+export const TodoList = () => {
+  const { todos, setTodos } = useContext(TodosContext);
+
+  const [filterParam, setFilterParam] = useState('All');


Create an enum for your filter types, don't pass just strings

BudnikOleksii · 2023-09-15T16:13:36Z

src/components/TodoList/TodoList.tsx

+    const todosCopy = [...todos].map(todo => ({
+      ...todo, completed: event.target.checked,
+    }));


map creates a new array, you don't need to create a copy.
Also, use functional setState

BudnikOleksii · 2023-09-15T16:15:54Z

src/components/TodoList/TodoList.tsx

+              {todos.filter(todo => {
+                switch (filterParam) {
+                  case ('All'):
+                    return true;
+                  case ('Active'):
+                    return !todo.completed;
+                  case ('Completed'):
+                    return todo.completed;
+                  default:
+                    return true;
+                }


Move it outside jsx, create a handler or a helper function for this, or just store filteredTodos in a variable(you can use useMemo for this variable if you want)

BudnikOleksii · 2023-09-15T16:16:33Z

src/components/TodosFilter/TodosFilter.tsx

+enum Status {
+  All = 'All',
+  Active = 'Active',
+  Completed = 'Completed',
+}


So you have the enum but don't use it in TodoList component 🤷‍♂️

roman-mirzoian

Be more attentive to the comments and if you update some part of the code, take a look, maybe you should remove something extra.

roman-mirzoian · 2023-09-18T14:04:48Z

src/TodosContext.tsx

+export const useTodos = () => {
+  const todos = useContext(TodosContext);
+
+  return todos;
+};


Suggested change

export const useTodos = () => {

const todos = useContext(TodosContext);

return todos;

};

export const useTodos = () => {

return useContext(TodosContext);

};

roman-mirzoian · 2023-09-18T14:07:39Z

src/components/TodoItem/TodoItem.tsx

+  const handleRemoveTodo = () => {
+    const todosCopy = [...todos];
+
+    todosCopy.splice(index, 1);
+
+    setTodos(prev => prev.filter(({ id }) => id !== todo.id));
+  };


#679 (comment) - not fully fixed

roman-mirzoian · 2023-09-18T14:09:27Z

src/components/TodoItem/TodoItem.tsx

+  const handleEditTodo = (
+    title: string,
+  ) => {
+    const todosCopy = [...todos];
+
+    if (title && title !== todo.title) {
+      todosCopy.splice(index, 1, { ...todo, title });
+
+      setTodos(prev => prev.map(currentTodo => {
+        if (currentTodo.id === todo.id) {
+          return { ...currentTodo, title };
+        }
+
+        return currentTodo;
+      }));


#679 (comment) - not fully fixed

roman-mirzoian · 2023-09-18T14:11:07Z

src/components/TodoList/TodoList.tsx

+  const filteredTodos = useMemo(() => {
+    return todos.filter(todo => {
+      switch (filterParam) {
+        case (Status.All):
+          return todo;
+        case (Status.Active):
+          return !todo.completed;
+        case (Status.Completed):
+          return todo.completed;
+        default:
+          return todo;
+      }
+    });
+  }, [filterParam, todos]);


#679 (comment) - not fully fixed

BudnikOleksii

Good work 🚀

BudnikOleksii · 2023-09-19T11:30:55Z

src/types/todo.ts

+export interface Todo {
+  id: string,
+  title: string,
+  completed: true | false,


Suggested change

completed: true | false,

completed: boolean,

boolean type is the same

davinnchii added 10 commits September 11, 2023 15:47

Implemented logic for adding the todo and changing its status

a1846d8

Implemented logic for toggleAll button

363e8bd

Implemented logic to swetch between active, completed and all todos

7c94408

Implemented logic to delete todos from list

bc12644

Added conditional rendering of TodoList component

ba095ea

Added logic for editing a todo

505aac1

Fix editing a todo, including onBlur event

70fa7cf

Implemented logic to use local storage instead of todos

8e2276c

Fixed tests

b98ffbb

Update README.md

c89b44c

BudnikOleksii suggested changes Sep 15, 2023

View reviewed changes

Fix comments

a48d8ea

davinnchii requested a review from BudnikOleksii September 16, 2023 15:53

Wesses approved these changes Sep 16, 2023

View reviewed changes

roman-mirzoian suggested changes Sep 18, 2023

View reviewed changes

davinnchii added 2 commits September 19, 2023 09:51

Fixed comments

090d063

Added filterTodos.ts

b7718c4

davinnchii requested a review from roman-mirzoian September 19, 2023 07:55

BudnikOleksii approved these changes Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created Todo app #679

Created Todo app #679

davinnchii commented Sep 12, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

BudnikOleksii Sep 15, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 18, 2023

roman-mirzoian Sep 18, 2023

roman-mirzoian Sep 18, 2023

roman-mirzoian Sep 18, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 19, 2023

		// [JSON.parse(localStorage.getItem('todos')
		// \|\| '{}')] as Todo[],

		setTodos((prev: Todo[]) => [...prev,
		{ id: uuidv4(), title: newTodoTitle, completed: false }]);

Created Todo app #679

Are you sure you want to change the base?

Created Todo app #679

Conversation

davinnchii commented Sep 12, 2023

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-mirzoian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment