Develop #815

Valentyn-Bilyk · 2023-09-27T06:36:21Z

BudnikOleksii

Good work 👍
Left some suggestions. Also, let's split logic, don't store all logic in the App component.
For example:

filter logic in the filter component
toggle all in header as the button in this component
add new todo in the form component or in the header too
and so on

The smaller your components, the easier it is to manage and support them.

BudnikOleksii · 2023-09-27T08:45:14Z

src/App.tsx

+  const timerId = useRef<number>(0);
+
+  useEffect(() => {
+    if (timerId.current) {
+      window.clearTimeout(timerId.current);
+    }
+
+    timerId.current = window.setTimeout(() => {
+      setErrorMessage(ErrorMessage.Default);
+    }, 3000);
+  }, [errorMessage]);


Move this logic to ErrorNotification component

BudnikOleksii · 2023-09-27T08:50:11Z

src/App.tsx

+      });
+  };
+
+  const handleUpdateTodo = async (todo: Todo, newTodoTitle: string) => {


If you don't use await there no sense to do your function async

BudnikOleksii · 2023-09-27T08:50:49Z

src/App.tsx

+      .updateTodo({
+        id: todo.id,
+        title: newTodoTitle,
+        userId: todo.userId,
+        completed: todo.completed,
+      })


Suggested change

.updateTodo({

id: todo.id,

title: newTodoTitle,

userId: todo.userId,

completed: todo.completed,

})

.updateTodo({

...todo,

title: newTodoTitle,

})

BudnikOleksii · 2023-09-27T08:52:16Z

src/App.tsx

+  const handleDeleteAllCompletedTodos = () => {
+    todos
+      .filter((todo) => todo.completed)
+      .forEach((todo) => handleDeleteTodo(todo.id));
+  };


When you need to send a batch of requests it's better to create an array of requests and handle them with Promise.all or Promise.allsettled

src/App.tsx

+    if (!isAllCompleted) {
+      todos
+        .filter((todo) => !todo.completed)
+        .forEach((todo) => handleCompletedChange(todo));
+    }
+
+    if (isAllCompleted) {
+      todos
+        .filter((todo) => todo.completed)
+        .forEach((todo) => handleCompletedChange(todo));
+    }


BudnikOleksii · 2023-09-27T08:54:09Z

src/components/TodoAppFooter/TodoAppFooter.tsx

+      {`${todosCounter} items left`}
+    </span>
+
+    {/* Active filter should have a 'selected' class */}


Remove comments

Remove comments

Don't leave unnecessary comments, remove all of them

BudnikOleksii · 2023-09-27T08:55:39Z

src/components/TodoAppHeader/TodoAppHeader.tsx

+      .catch(() => {
+
+      });


Why is the catch block empty? You should notify the user if any error occurred.

Why is the catch block empty? You should notify the user if any error occurred.

🤷‍♂️

BudnikOleksii · 2023-09-27T08:56:29Z

src/components/TodoAppRow/TempTodo.tsx

+  todo: Todo,
+};
+
+export const TempTodo: React.FC<Props> = ({ todo }) => (


You have a TodoItem component with the same design, use it.
As in your case if you have any design changes you have to keep in mind and change in two components.

BudnikOleksii · 2023-09-27T08:59:42Z

src/components/TodoAppRow/TodoAppRow.tsx

+        await onTodoUpdate(todoTitle);
+        setIsEditing(false);
+      } catch (er) {
+        console.error('catch error');


You should notify the user in case of an error instead of console.error

Not fixed 😢

BudnikOleksii · 2023-09-27T09:01:46Z

src/utils/TodoFilter.ts

+export enum FilterLink {
+  All = '',
+  Active = 'active',
+  Completed = 'completed',
+}


Let's move to the types folder

Let's move to the types folder

Not fixed

BudnikOleksii

Some of the comments are without any fixes and replies, check them

BudnikOleksii · 2023-10-03T10:33:02Z

src/App.tsx

+    return todoService
+      .addTodo(todoTitle)
+      .then((newTodo) => {
+        setTodos((prevTodos) => [...prevTodos, newTodo]);
+      })
+      .catch((error) => {
+        setErrorMessage(ErrorMessage.Add);
+        throw error;


You don't need to return something from the handler and throw an error as you handle it on this step

BudnikOleksii · 2023-10-03T10:34:21Z

src/App.tsx

+        setErrorMessage(ErrorMessage.Update);
+        throw error;


The same, and in all other places

BudnikOleksii · 2023-10-03T10:37:33Z

src/App.tsx

+    if (!isAllCompleted) {
+      todos
+        .filter((todo) => !todo.completed)
+        .forEach((todo) => handleCompletedChange(todo));
+    }
+
+    if (isAllCompleted) {
+      todos
+        .filter((todo) => todo.completed)
+        .forEach((todo) => handleCompletedChange(todo));
+    }


Suggested change

if (!isAllCompleted) {

todos

.filter((todo) => !todo.completed)

.forEach((todo) => handleCompletedChange(todo));

}

if (isAllCompleted) {

todos

.filter((todo) => todo.completed)

.forEach((todo) => handleCompletedChange(todo));

}

todos

.filter((todo) => todo.completed === isAllCompleted)

.forEach((todo) => handleCompletedChange(todo));

You can simplify in this case

BudnikOleksii · 2023-10-03T10:47:46Z

src/components/TodoAppFooter/TodoAppFooter.tsx

+      {`${todosCounter} items left`}
+    </span>
+
+    {/* Active filter should have a 'selected' class */}


Remove comments

Don't leave unnecessary comments, remove all of them

BudnikOleksii · 2023-10-03T10:50:17Z

src/components/TodoAppHeader/TodoAppHeader.tsx

+      .catch(() => {
+
+      });


Why is the catch block empty? You should notify the user if any error occurred.

🤷‍♂️

BudnikOleksii · 2023-10-03T10:50:42Z

src/components/TodoAppRow/TodoAppRow.tsx

+      } catch (er) {
+        // console.error('catch error');
+      }


🤷‍♂️

BudnikOleksii · 2023-10-03T10:51:27Z

src/utils/TodoFilter.ts

+export enum FilterLink {
+  All = '',
+  Active = 'active',
+  Completed = 'completed',
+}


Let's move to the types folder

Not fixed

Valentyn-Bilyk · 2023-10-04T12:28:42Z

As for me, it's okay when all handler logic is in one file in this task.

roman-mirzoian

Good job, but there is quite a bit of work to be done.

roman-mirzoian · 2023-10-04T15:35:16Z

src/App.tsx

@@ -1,24 +1,190 @@
-/* eslint-disable max-len */
 /* eslint-disable jsx-a11y/control-has-associated-label */


Try to rewrite the code so that it is not necessary to disable the linter

roman-mirzoian · 2023-10-04T15:36:40Z

src/App.tsx

+      .filter((todo) => todo.completed)
+      .forEach((todo) => handleDeleteTodo(todo.id));


It is not necessary to go around the array twice, it is enough to move the filtering condition to the middle of the forEach and go through the array once instead of twice

roman-mirzoian · 2023-10-04T15:37:08Z

src/App.tsx

+    return todos.filter(todo => !todo.completed).length;
+  }, [todos]);
+
+  const isAllCompleted = useMemo(() => {


Suggested change

const isAllCompleted = useMemo(() => {

const isAllTodosCompleted = useMemo(() => {

roman-mirzoian · 2023-10-04T15:37:32Z

src/App.tsx

+      .filter((todo) => todo.completed === isAllCompleted)
+      .forEach((todo) => handleCompletedChange(todo));


It is not necessary to go around the array twice, it is enough to move the filtering condition to the middle of the forEach and go through the array once instead of twice

roman-mirzoian · 2023-10-04T15:38:02Z

src/components/ErrorNotification/ErrorNotification.tsx

+  setErrorMessage,
+  errorMessage,
+}) => {
+  const timerId = useRef<number>(0);


Suggested change

const timerId = useRef<number>(0);

const timerIdRef = useRef<number>(0);

roman-mirzoian · 2023-10-04T15:39:02Z

src/components/TodoAppHeader/TodoAppHeader.tsx

+  const onFormSubmit = (event: React.FormEvent<HTMLFormElement>) => {
+    event.preventDefault();
+
+    if (todoTitle.trim().length === 0) {


Suggested change

if (todoTitle.trim().length === 0) {

if (!todoTitle.trim().length) {

roman-mirzoian · 2023-10-04T15:39:24Z

src/components/TodoAppHeader/TodoAppHeader.tsx

+            active: isAllCompleted,
+          })}
+          data-cy="ToggleAllButton"
+          onClick={() => onToggleClick()}


Suggested change

onClick={() => onToggleClick()}

onClick={onToggleClick}

roman-mirzoian · 2023-10-04T15:39:36Z

src/components/TodoAppHeader/TodoAppHeader.tsx

+      <form
+        onSubmit={onFormSubmit}
+      >


Suggested change

<form

onSubmit={onFormSubmit}

>

<form onSubmit={onFormSubmit}>

roman-mirzoian · 2023-10-04T15:40:09Z

src/components/TodoAppRow/TodoAppRow.tsx

+        await onTodoUpdate(todoTitle);
+        setIsEditing(false);
+      } catch (er) {
+        console.error('catch error');


Not fixed 😢

roman-mirzoian · 2023-10-04T15:41:37Z

src/utils/TodoFilter.ts

+  arr: Todo[],
+  option: string,
+) {
+  const todos = [...arr];


The filter method returns a copy of the array, so you don't have to do it manually

BudnikOleksii

Good work 👍

Valentyn-Bilyk added 2 commits September 26, 2023 19:22

test

7b56f1e

have some problem with toggle button

f8f0108

BudnikOleksii suggested changes Sep 27, 2023

View reviewed changes

all handler in 1 file, i think it`s good

2b72df7

Valentyn-Bilyk requested a review from BudnikOleksii October 1, 2023 17:03

BudnikOleksii suggested changes Oct 3, 2023

View reviewed changes

fix

9d65ab0

Valentyn-Bilyk requested a review from BudnikOleksii October 4, 2023 12:28

roman-mirzoian suggested changes Oct 4, 2023

View reviewed changes

complete

094fa4e

Valentyn-Bilyk requested a review from roman-mirzoian October 5, 2023 07:21

BudnikOleksii approved these changes Oct 5, 2023

View reviewed changes

		@@ -1,24 +1,190 @@
		/* eslint-disable max-len */
		/* eslint-disable jsx-a11y/control-has-associated-label */

		.filter((todo) => todo.completed)
		.forEach((todo) => handleDeleteTodo(todo.id));

	const isAllCompleted = useMemo(() => {
	const isAllTodosCompleted = useMemo(() => {

		.filter((todo) => todo.completed === isAllCompleted)
		.forEach((todo) => handleCompletedChange(todo));

	const timerId = useRef<number>(0);
	const timerIdRef = useRef<number>(0);

	if (todoTitle.trim().length === 0) {
	if (!todoTitle.trim().length) {

Develop #815

Are you sure you want to change the base?

Develop #815

Conversation

Valentyn-Bilyk commented Sep 27, 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

This comment was marked as outdated.

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

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

Valentyn-Bilyk commented Oct 4, 2023

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

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

BudnikOleksii left a comment

Choose a reason for hiding this comment