Implement todo update feature #798

OlhaMomot · 2023-09-26T07:35:12Z

BudnikOleksii

Good work 👍
Left some suggestions, check them.
Also, show to user what todos are updating when user clicks the toggle all button.

BudnikOleksii · 2023-09-26T13:40:53Z

src/App.tsx

+  useEffect(() => {
+    let timer: NodeJS.Timeout | undefined;
+
+    if (errorMessage) {
+      timer = setTimeout(() => {
+        setErrorMessage('');
+      }, 3000);
+    }
+
+    return () => {
+      if (timer) {
+        clearTimeout(timer);
+      }
+    };
+  }, [errorMessage]);


Let's create a component ErrorNotification and move this logic and UI to it

BudnikOleksii · 2023-09-26T13:46:14Z

src/App.tsx

+  const onToggleAll = async () => {
+    if (activeTodos.length) {
+      activeTodos.forEach(
+        currentTodo => updateTodoHandler(currentTodo, { completed: true }),
+      );
+    } else {
+      todos.forEach(
+        currentTodo => updateTodoHandler(currentTodo, { completed: false }),
+      );
+    }
+  };


It's better to create an array of requests and then handle them with Promise.all.
Also, show to user which todos are updating, add a loader to them

BudnikOleksii · 2023-09-26T13:46:37Z

src/App.tsx

+        <header className="todoapp__header">
+          {!!todos.length
+            && (
+              <button
+                type="button"
+                className={classNames(
+                  'todoapp__toggle-all',
+                  { active: !activeTodos.length },
+                )}
+                data-cy="ToggleAllButton"
+                onClick={onToggleAll}
+              />
+            )}
+
+          <Form setTempTodo={setTempTodo} />
+        </header>


Create a component Header

BudnikOleksii · 2023-09-26T13:47:06Z

src/App.tsx

+      <div
+        data-cy="ErrorNotification"


As I mentioned before, create a component ErrorNotification

BudnikOleksii · 2023-09-26T13:48:21Z

src/components/Footer.tsx

+type Props = {
+  activeTodos: Todo[],
+  filter: string,
+  FilterOption: { [key: string]: string },


You have enum FilterOption, use it instead

BudnikOleksii · 2023-09-26T13:52:29Z

src/components/TodoItem.tsx

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


Suggested change

export const TodoItem: React.FC<Props> = (

{ todo }: Props,

) => {

export const TodoItem: React.FC<Props> = ({ todo }) => {

You have already set Props type in the generic

BudnikOleksii · 2023-09-26T13:55:33Z

src/components/TodoItem.tsx

+    } else if (todoTitle === todo.title) {
+      setIsEditing(false);
+
+      return;


If todoTitle is the same we have an infinite spinner as you set it previously setIsItemLoading(true); and just return in this case

BudnikOleksii · 2023-09-26T13:57:21Z

src/components/TodoItem.tsx

+    event.preventDefault();
+    setIsItemLoading(true);
+
+    if (todoTitle !== todo.title && todoTitle !== '') {


You compare todoTitle !== todo.title it two times, create a variable.
Also, you don't need to compare with an empty string, just check if todoTitle exists.
https://developer.mozilla.org/en-US/docs/Glossary/Truthy

BudnikOleksii · 2023-09-26T13:58:17Z

src/components/TodoList.tsx

+      {tempTodo
+        && <TodoItem todo={tempTodo} />}


Suggested change

{tempTodo

&& <TodoItem todo={tempTodo} />}

{tempTodo && (

<TodoItem todo={tempTodo} />

)}

BudnikOleksii · 2023-09-26T14:00:11Z

src/components/TodoProvider.tsx

+  const [isLoadingMap, setIsLoadingMap]
+    = useState<{ [key: number]: boolean } | {}>({});


You can just store an array of loading todos ids and it will be a bit easy to manage

…ed other problems

Moroz-Dmytro

When I rename todo to empty(enter just spaces) -> todo should be deleted

"1 items left", but all todos are checked

Moroz-Dmytro · 2023-09-29T08:40:32Z

src/App.tsx

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


I would move this logic to separate helper function

Moroz-Dmytro · 2023-09-29T08:41:03Z

src/App.tsx

+        {!!filteredTodos.length
+          && <TodoList todos={filteredTodos} tempTodo={tempTodo} />}


Suggested change

{!!filteredTodos.length

&& <TodoList todos={filteredTodos} tempTodo={tempTodo} />}

{!!filteredTodos.length && (

<TodoList todos={filteredTodos} tempTodo={tempTodo} />

)}

Moroz-Dmytro · 2023-09-29T08:41:20Z

src/App.tsx

+        {!!todos.length
+          && (
+            <Footer
+              activeTodos={activeTodos}
+              filter={filter}
+              setFilter={setFilter}
+            />
+          )}


Suggested change

{!!todos.length

&& (

<Footer

activeTodos={activeTodos}

filter={filter}

setFilter={setFilter}

/>

)}

{!!todos.length && (

<Footer

activeTodos={activeTodos}

filter={filter}

setFilter={setFilter}

/>

)}

Moroz-Dmytro · 2023-09-29T08:41:29Z

src/App.tsx

+      </div>
+      <ErrorNotification


Suggested change

</div>

<ErrorNotification

</div>

<ErrorNotification

Moroz-Dmytro · 2023-09-29T08:42:37Z

src/components/Footer.tsx

+  const completedTodos = useMemo(() => {
+    return todos.filter(({ completed }) => completed === true);
+  }, [todos]);


Suggested change

const completedTodos = useMemo(() => {

return todos.filter(({ completed }) => completed === true);

}, [todos]);

const completedTodos = todos.filter(({ completed }) => completed);

useMemo is useless here

Moroz-Dmytro · 2023-09-29T08:44:17Z

src/components/Footer.tsx

+      .map(({ id }) => deleteTodoHandler(id)));
+  };
+
+  const isClearButtonInvisible = completedTodos.length === 0;


Suggested change

const isClearButtonInvisible = completedTodos.length === 0;

const isClearButtonInvisible = !completedTodos.length;

But you don't really need variable for this

Moroz-Dmytro · 2023-09-29T08:45:19Z

src/components/Header.tsx

+    if (activeTodos.length) {
+      Promise.all(activeTodos
+        .map(currentTodo => updateTodoHandler(
+          currentTodo,
+          { completed: true },
+        )));
+    } else {
+      Promise.all(todos
+        .map(currentTodo => updateTodoHandler(
+          currentTodo,
+          { completed: false },
+        )));
+    }


DRY. a lot of code repeats, try to refactor this

Also add await before Promise.all

Moroz-Dmytro · 2023-09-29T08:46:38Z

src/components/TodoProvider.tsx

+  const [errorMessage, setErrorMessage] = useState('');
+  const [isLoadingTodoIds, setIsLoadingTodoIds] = useState<number[]>([]);
+
+  const addTodoHandler = async (


Suggested change

const addTodoHandler = async (

const handleAddTodo = async (

The same all other names

Moroz-Dmytro · 2023-09-29T08:47:54Z

src/components/TodoProvider.tsx

+
+      setTodos((currentTodos: Todo[]) => {
+        return currentTodos.map((currentTodo) => {
+          return currentTodo.id === updatedTodo.id ? updatedTodo : currentTodo;


Suggested change

return currentTodo.id === updatedTodo.id ? updatedTodo : currentTodo;

return currentTodo.id === updatedTodo.id

? updatedTodo

: currentTodo;

Moroz-Dmytro · 2023-09-29T08:48:59Z

src/components/TodoItem.tsx

+            'is-active': (isLoadingTodoIds.includes(todo.id))
+              || isItemLoading,


Move this to vatiable

dffUqp

LGTM:fire: but needs a few minor fixes:smiley:

dffUqp · 2023-09-29T14:53:13Z

src/components/TodoItem.tsx

+  document.addEventListener('keyup', (e) => {
+    if (e.key === 'Escape') {
+      setIsEditing(false);
+    }
+  });


Strange situations with this code, It works even if I don't have any todos on the page

I think this article can help you to solve this problem
https://www.freecodecamp.org/news/react-useeffect-absolute-beginners/#:~:text=What%20are%20side%20effects%20in,give%20us%20a%20predictable%20result.

2023-09-29.17-48-55.mov

dffUqp · 2023-09-29T14:54:42Z

src/components/TodoItem.tsx

+    handleUpdateTodo,
+  } = useTodo();
+
+  const onCheckboxChange = async () => {


It is just a recomendation:

Component events start with the prefix on but the function that handles these events should be named with the handle prefix

dffUqp · 2023-09-29T14:56:54Z

src/components/Header.tsx

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


let's fix this eslint issue:smiley:

try to work around aria-label , I think it can help

dffUqp · 2023-09-29T15:07:54Z

src/components/Header.tsx

+  useEffect(() => {
+    if (todoTitleField.current) {
+      todoTitleField.current.focus();
+    }
+  });


useEffect without a dependency array, is a bad practice

Currently, this useEffect triggers on every Header render, try to look at how it works on the mate demo.

I think there they focus input only after delete or add todo (after changing the amount of todos)

dffUqp · 2023-09-29T15:12:34Z

src/components/TodoItem.tsx

+        ? (
+          <form
+            onSubmit={onTodoSave}
+            onBlur={onTodoSave}


I know that a lot of tests is not valid, but in your case you have some valid tests failed

For example:

15) Renaming Edit Form on enter before recieved a response should stay while waiting:

on click 5) should send requests only for not completed todos

dffUqp · 2023-09-29T15:13:20Z

src/components/TodoList.tsx

+      {todos.map(todo => {
+        return (
+          <TodoItem
+            todo={todo}
+            key={todo.id}
+          />
+        );
+      })}


Let's remove return keyword here

dffUqp · 2023-09-29T15:14:42Z

src/hooks/useTodo.tsx

+  const {
+    todos,
+    handleAddTodo,
+    handleDeleteTodo,
+    handleUpdateTodo,
+    errorMessage,
+    setErrorMessage,
+    isLoadingTodoIds,
+  } = useContext(TodoContext);
+
+  return {
+    todos,
+    handleAddTodo,
+    handleDeleteTodo,
+    handleUpdateTodo,
+    errorMessage,
+    setErrorMessage,
+    isLoadingTodoIds,
+  };


Will it work?

Suggested change

const {

todos,

handleAddTodo,

handleDeleteTodo,

handleUpdateTodo,

errorMessage,

setErrorMessage,

isLoadingTodoIds,

} = useContext(TodoContext);

return {

todos,

handleAddTodo,

handleDeleteTodo,

handleUpdateTodo,

errorMessage,

setErrorMessage,

isLoadingTodoIds,

};

const todoContextValues = useContext(TodoContext);

return todoContextValues

roman-mirzoian

Well done 👍

roman-mirzoian · 2023-10-06T11:55:12Z

src/components/ErrorNotification.tsx

+      data-cy="ErrorNotification"
+      className={
+        classNames(
+          'notification is-danger is-light has-text-weight-normal',


Suggested change

'notification is-danger is-light has-text-weight-normal',

'notification', 'is-danger', 'is-light', 'has-text-weight-normal',

roman-mirzoian · 2023-10-06T11:56:47Z

src/components/TodoItem.tsx

+      <div
+        data-cy="TodoLoader"
+        className={classNames(
+          'modal overlay',


Suggested change

'modal overlay',

'modal', 'overlay',

OlhaMomot added 2 commits September 26, 2023 00:32

implemented todo update feature

0cc9fe9

fixed updating empty todo

6c3ebfd

BudnikOleksii suggested changes Sep 26, 2023

View reviewed changes

created header and errorNotification components, useTodo hook and fix…

c7bd252

…ed other problems

OlhaMomot requested a review from BudnikOleksii September 28, 2023 12:30

Moroz-Dmytro suggested changes Sep 29, 2023

View reviewed changes

fixed bug with empty title & improved code readability

ed445ff

OlhaMomot requested a review from Moroz-Dmytro September 29, 2023 13:33

dffUqp suggested changes Sep 29, 2023

View reviewed changes

fixed toggleAll and escape functions

3354fc4

roman-mirzoian approved these changes Oct 6, 2023

View reviewed changes

		const [isLoadingMap, setIsLoadingMap]
		= useState<{ [key: number]: boolean } \| {}>({});

		{!!filteredTodos.length
		&& <TodoList todos={filteredTodos} tempTodo={tempTodo} />}

	const isClearButtonInvisible = completedTodos.length === 0;
	const isClearButtonInvisible = !completedTodos.length;

		'is-active': (isLoadingTodoIds.includes(todo.id))
		\|\| isItemLoading,

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

	'notification is-danger is-light has-text-weight-normal',
	'notification', 'is-danger', 'is-light', 'has-text-weight-normal',

Implement todo update feature #798

Are you sure you want to change the base?

Implement todo update feature #798

Conversation

OlhaMomot commented Sep 26, 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

Moroz-Dmytro 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dffUqp 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

dffUqp Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

dffUqp Sep 29, 2023 • edited Loading

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

dffUqp Sep 29, 2023 •

edited

Loading

dffUqp Sep 29, 2023 •

edited

Loading