add task solution #812

yar0man · 2023-09-27T05:35:52Z

Denys14Samoilenko

Good work! 🔥
I leave some comments for you!

Denys14Samoilenko · 2023-09-28T09:15:45Z

src/App.tsx

+      .catch(() => {
+        setErrorMesssage('Unable to load todos');
+
+        setTimeout(() => {


I think you need to clear this timeout

Denys14Samoilenko · 2023-09-28T09:18:19Z

src/components/ErrorNotification/ErrorNotification.tsx

+        data-cy="HideErrorButton"
+        type="button"
+        className="delete"
+        onClick={() => onErrorMesssageChange('')}


You can use your const removeErrorMessage here i think

Good point.

roman-mirzoian

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

roman-mirzoian · 2023-09-28T21:31:18Z

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-28T21:31:42Z

src/App.tsx

+
+  const [title, setTitle] = useState('');
+  const [tempTodo, setTempTodo] = useState<Todo | null>(null);
+  const [isDisable, setIsDisable] = useState(false);


Suggested change

const [isDisable, setIsDisable] = useState(false);

const [isDisabled, setIsDisabled] = useState(false);

roman-mirzoian · 2023-09-28T21:32:23Z

src/App.tsx

+          onErrorMesssageChange={setErrorMesssage}
+        />
+
+        {filteredTodos.length > 0 && (


Suggested change

{filteredTodos.length > 0 && (

{!!filteredTodos.length && (

roman-mirzoian · 2023-09-28T21:32:34Z

src/App.tsx

+          />
+        )}
+
+        {todos.length > 0 && (


Suggested change

{todos.length > 0 && (

{!!todos.length && (

roman-mirzoian · 2023-09-28T21:32:42Z

src/App.tsx

+            onSelectedFilter={setSelectedFilter}
+          />
+        )}
+


Suggested change

roman-mirzoian · 2023-09-28T21:34:35Z

src/components/TodoFilter/TodoFilter.tsx

+  return (
+    <footer className="todoapp__footer" data-cy="Footer">
+      <span className="todo-count" data-cy="TodosCounter">
+        {`${countTodos(todos, false)} items left`}


If there is one item, there should not be a plural in the text.

roman-mirzoian · 2023-09-28T21:35:10Z

src/components/TodoHeader/TodoHeder.tsx

@@ -0,0 +1,102 @@
+/* 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-28T21:36:15Z

src/components/TodoItem/TodoItem.tsx

@@ -0,0 +1,162 @@
+/* eslint-disable max-len */


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

roman-mirzoian · 2023-09-28T21:37:48Z

src/components/TodoItem/TodoItem.tsx

+  };
+
+  const updateTitle = () => {
+    if (editedTitle.trim() === todo.title) {


You use editedTitle.trim() three times, it's better to put it in a variable and reuse it.

roman-mirzoian · 2023-09-28T21:38:01Z

src/components/TodoItem/TodoItem.tsx

+      data-cy="Todo"
+      key={todo.id}
+      className={classNames('todo', {
+        completed: todo.completed === true,


Suggested change

completed: todo.completed === true,

completed: todo.completed

BudnikOleksii

Good work 👍
Left a few suggestons

BudnikOleksii · 2023-09-29T08:33:02Z

src/App.tsx

+    setTempTodo({
+      id: 0,
+      userId: USER_ID,
+      title: title.trim(),
+      completed: false,
+    });
+
+    setIsDisabled(true);
+
+    addTodo({
+      userId: USER_ID,
+      title: title.trim(),
+      completed: false,
+    })


Suggested change

setTempTodo({

id: 0,

userId: USER_ID,

title: title.trim(),

completed: false,

});

setIsDisabled(true);

addTodo({

userId: USER_ID,

title: title.trim(),

completed: false,

})

const newTodo = {

userId: USER_ID,

title: title.trim(),

completed: false,

};

setTempTodo({

...newTodo,

id: 0,

});

setIsDisabled(true);

addTodo(newTodo)

Don't repeat yourself

BudnikOleksii · 2023-09-29T08:34:33Z

src/App.tsx

+        setTimeout(() => {
+          setErrorMesssage('');
+        }, 3000);


You can trigger timeout in the ErrorNotification component in case of any error

BudnikOleksii · 2023-09-29T08:35:45Z

src/components/TodoFilter/TodoFilter.tsx

+  return (
+    <footer className="todoapp__footer" data-cy="Footer">
+      <span className="todo-count" data-cy="TodosCounter">
+        {`${countTodos(todos, false)} ${countTodos(todos, false) === 1 ? 'item' : 'items'} left`}


countTodos(todos, false) You do it twice here, create a variable

BudnikOleksii · 2023-09-29T08:41:23Z

src/utils/filterTodos.ts

+  let todosCopy = [...todos];
+
+  if (selectedFilter) {
+    todosCopy = todosCopy.filter(({ completed }) => {
+      switch (selectedFilter) {
+        case FilterStatus.Active:
+          return !completed;
+        case FilterStatus.Completed:
+          return completed;
+        default:
+          return todosCopy;


In the default case, you get an array of todosCopy arrays. And filter creates a new array so you don't need to create copy in this case

add task solution

5213748

Denys14Samoilenko approved these changes Sep 28, 2023

View reviewed changes

roman-mirzoian suggested changes Sep 28, 2023

View reviewed changes

add some changes

e6e8670

BudnikOleksii approved these changes Sep 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add task solution #812

add task solution #812

yar0man commented Sep 27, 2023

Denys14Samoilenko left a comment

Denys14Samoilenko Sep 28, 2023

Denys14Samoilenko Sep 28, 2023

roman-mirzoian Sep 28, 2023

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

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 29, 2023

BudnikOleksii Sep 29, 2023

BudnikOleksii Sep 29, 2023

BudnikOleksii Sep 29, 2023

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

	const [isDisable, setIsDisable] = useState(false);
	const [isDisabled, setIsDisabled] = useState(false);

		@@ -0,0 +1,102 @@
		/* eslint-disable jsx-a11y/control-has-associated-label */

	completed: todo.completed === true,
	completed: todo.completed

add task solution #812

Are you sure you want to change the base?

add task solution #812

Conversation

yar0man commented Sep 27, 2023

Denys14Samoilenko 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

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