add rename todo and completed status todo #807

Denys14Samoilenko · 2023-09-26T19:17:13Z

roman-mirzoian

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

roman-mirzoian · 2023-09-26T21:10:24Z

src/App.tsx

 /* 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-09-26T21:11:18Z

src/App.tsx

+      .then(setTodos)
+      .catch((error) => {
+        setErrorMessage(TodosError.DOWNLOAD_ERROR_MESSAGE);
+        throw error;


Suggested change

throw error;

throw Error(error.message);

Fix this in all code.

roman-mirzoian · 2023-09-26T21:11:26Z

src/App.tsx

+      });
+  }, []);
+
+  const timerId = useRef<number>(0);


Suggested change

const timerId = useRef<number>(0);

const timerIdRef = useRef<number>(0);

roman-mirzoian · 2023-09-26T21:11:35Z

src/App.tsx

+  useEffect(() => {
+    if (timerId.current) {
+      window.clearTimeout(timerId.current);
+    }
+
+    timerId.current = window.setTimeout(() => {
+      setErrorMessage('');
+    }, 3000);
+  }, [errorMessage]);


https://upmostly.com/tutorials/settimeout-in-react-components-using-hooks

roman-mirzoian · 2023-09-26T21:13:17Z

src/App.tsx

+      <h1 className="todoapp__title">todos</h1>
+
+      <div className="todoapp__content">
+


Suggested change

roman-mirzoian · 2023-09-26T21:17:02Z

src/components/Header/Header.tsx

+
+  return (
+    <header className="todoapp__header">
+      {/* this buttons is active only if there are some active todos */}


Do not leave unnecessary comments.

roman-mirzoian · 2023-09-26T21:17:07Z

src/components/Header/Header.tsx

+        className="todoapp__toggle-all active"
+        data-cy="ToggleAllButton"
+      />
+      {/* Add a todo on form submit */}


Do not leave unnecessary comments.

roman-mirzoian · 2023-09-26T21:17:26Z

src/components/Loader/Loader.tsx

+        'modal',
+        'overlay',
+        {
+          'is-active': isProcessing || todoId === 0,


Suggested change

'is-active': isProcessing || todoId === 0,

'is-active': isProcessing || !todoId,

roman-mirzoian · 2023-09-26T21:17:43Z

src/components/TodoItem/TodoItem.tsx

+    }
+  };
+
+  const titleInput = useRef<HTMLInputElement | null>(null);


Suggested change

const titleInput = useRef<HTMLInputElement | null>(null);

const titleInputRef = useRef<HTMLInputElement | null>(null);

roman-mirzoian · 2023-09-26T21:18:04Z

src/components/TodoItem/TodoItem.tsx

+              onClick={() => {
+                onDeleteTodo();
+              }}


Suggested change

onClick={() => {

onDeleteTodo();

}}

onClick={onDeleteTodo}

Vayts

well done!

roman-mirzoian

Almost done.

roman-mirzoian · 2023-09-28T22:11:40Z

src/App.tsx

+    if (isAllCompleted) {
+      Promise.all(visibleTodos.map(updateTodoStatus));
+    } else {
+      Promise.all(activeTodos.map(updateTodoStatus));
+    }


"DRY"

Suggested change

if (isAllCompleted) {

Promise.all(visibleTodos.map(updateTodoStatus));

} else {

Promise.all(activeTodos.map(updateTodoStatus));

}

const todosForUpdate = isAllCompleted ? visibleTodos : activeTodos;

Promise.all(todosForUpdate.map(updateTodoStatus));

roman-mirzoian · 2023-09-28T22:12:03Z

src/App.tsx

+        />
+
+        <section className="todoapp__main" data-cy="TodoList">
+


Not fixed 😢

roman-mirzoian · 2023-09-28T22:12:55Z

src/components/Footer/Footer.tsx

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


Not fixed 😢

roman-mirzoian · 2023-09-28T22:13:12Z

src/components/Footer/Footer.tsx

+        type="button"
+        className="todoapp__clear-completed"
+        data-cy="ClearCompletedButton"
+        style={{ visibility: completedTodosLength ? 'visible' : 'hidden' }}


Nit fixed 😢

ArtemTeslenko

There are some not fixed issues. Please, pay attention))

ArtemTeslenko · 2023-09-29T06:11:43Z

src/App.tsx

+      .then(setTodos)
+      .catch((error) => {
+        setErrorMessage(TodosError.DOWNLOAD_ERROR_MESSAGE);
+        throw Error(error.message);


it seems that your error message is in 'errorMessage', which you set on previous line, also add new 'throw new Error'

We can pass a pure error to the constructor of the Error class to see more information in the console

ArtemTeslenko · 2023-09-29T06:33:40Z

src/App.tsx

+      })
+      .catch((error) => {
+        setErrorMessage(TodosError.ADD_ERROR_MESSAGE);
+        throw Error(error.message);


See comment above.

roman-mirzoian

Almost done.
Now, if you update the title with only spaces, it will not be deleted, but will remain empty.
Also now, when there is only one item left, the inscription is displayed in the plural 1 items:
https://gyazo.com/da8bbe584d9275ad69b48843c104f688

roman-mirzoian · 2023-09-29T16:19:18Z

src/App.tsx

+      .then(setTodos)
+      .catch((error) => {
+        setErrorMessage(TodosError.DOWNLOAD_ERROR_MESSAGE);
+        throw Error(error.message);


We can pass a pure error to the constructor of the Error class to see more information in the console

roman-mirzoian · 2023-09-29T16:21:29Z

src/App.tsx

+      .then(() => {
+        setTodoTitle('');
+      })
+      .catch();


In the catch block, we must work with the error and not leave it empty

BudnikOleksii

Good work 👍

BudnikOleksii · 2023-10-05T11:22:08Z

src/App.tsx

+  const timerIdRef = useRef<number>(0);
+
+  useEffect(() => {
+    timerIdRef.current = window.setTimeout(() => {
+      setErrorMessage('');
+    }, 3000);
+
+    return () => clearTimeout(timerIdRef.current);
+  }, [errorMessage]);


You can keep related logic in the ErrorNotification component

BudnikOleksii · 2023-10-05T11:24:46Z

src/components/Footer/Footer.tsx

+        {todosSortBy === TodosSortType.All
+          ? `${todosLength - completedTodosLength} items left`
+          : `${todosLength} items left`}


It is better to put the number calculation in a separate variable for better readability.

Don't forget about all comments

add rename todo and completed status todo

8cd63af

roman-mirzoian suggested changes Sep 26, 2023

View reviewed changes

Denys14Samoilenko added 2 commits September 27, 2023 11:58

fixed variables names and errors, add toggleAll button logic

26cbc67

fixed linter error about button without label

c17d88b

Denys14Samoilenko requested a review from roman-mirzoian September 27, 2023 09:14

Vayts approved these changes Sep 28, 2023

View reviewed changes

roman-mirzoian reviewed Sep 28, 2023

View reviewed changes

ArtemTeslenko suggested changes Sep 29, 2023

View reviewed changes

fix formatting

fd2451b

Denys14Samoilenko requested a review from roman-mirzoian September 29, 2023 12:31

roman-mirzoian suggested changes Sep 29, 2023

View reviewed changes

fixed errors and adding todo with empty title when todo updated

9d46f10

Denys14Samoilenko requested a review from roman-mirzoian October 4, 2023 18:48

BudnikOleksii approved these changes Oct 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rename todo and completed status todo #807

add rename todo and completed status todo #807

Denys14Samoilenko commented Sep 26, 2023 •

edited

Loading

roman-mirzoian left a comment

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

Vayts left a comment

roman-mirzoian left a comment

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

ArtemTeslenko left a comment

ArtemTeslenko Sep 29, 2023

roman-mirzoian Sep 29, 2023

ArtemTeslenko Sep 29, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

BudnikOleksii left a comment

BudnikOleksii Oct 5, 2023

BudnikOleksii Oct 5, 2023

		/* eslint-disable max-len */
		/* eslint-disable jsx-a11y/control-has-associated-label */

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

		<h1 className="todoapp__title">todos</h1>

		<div className="todoapp__content">

	'is-active': isProcessing \|\| todoId === 0,
	'is-active': isProcessing \|\| !todoId,

	const titleInput = useRef<HTMLInputElement \| null>(null);
	const titleInputRef = useRef<HTMLInputElement \| null>(null);

add rename todo and completed status todo #807

Are you sure you want to change the base?

add rename todo and completed status todo #807

Conversation

Denys14Samoilenko commented Sep 26, 2023 • edited Loading

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

Vayts left a 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

ArtemTeslenko 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

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

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

Denys14Samoilenko commented Sep 26, 2023 •

edited

Loading