implement update logic, failed from 2 to 5 tests, every time different. Work in progress #806

KatOlista · 2023-09-26T18:48:57Z

FrontKid

Good job!

BudnikOleksii

Looks good 🚀
Left some comments to improve your suggestion. Check them and fix the bug when editing todo is empty it deletes todo, but then it tries to update it.

BudnikOleksii · 2023-09-28T09:48:32Z

src/components/Footer/Footer.tsx

+enum FilterOption {
+  Default = '',
+  All = 'All',
+  Active = 'Active',
+  Completed = 'Completed',
+}


Move types to the types folder

BudnikOleksii · 2023-09-28T09:48:58Z

src/components/Footer/Footer.tsx

+function filterTodos(option: FilterOption, todos: Todo[]) {
+  const filteredTodos = todos.filter(todo => {


Move to utils folder

BudnikOleksii · 2023-09-28T09:49:55Z

src/components/Footer/Footer.tsx

+
+const OPTIONS = [FilterOption.All, FilterOption.Completed, FilterOption.Active];
+
+export const Footer: React.FC<Props> = () => {


Suggested change

export const Footer: React.FC<Props> = () => {

export const Footer = () => {

If you don't have any props the types for them are redundant

BudnikOleksii · 2023-09-28T09:52:07Z

src/components/Footer/Footer.tsx

+  useEffect(() => {
+    setFilteredTodos(filterTodos(selectedOption, todos));
+    setHasCompleted(todos.some(todo => todo.completed));
+  }, [selectedOption, isTodoChange, changingItems]);
+
+  const handleFilterTodos = (option: FilterOption) => {
+    setFilteredTodos(filterTodos(option, todos));
+    setSelectedOption(option);
+  };


You do setFilteredTodos in this case twice.

BudnikOleksii · 2023-09-28T09:54:25Z

src/components/Header/Header.tsx

+    setTempTodo({
+      id: 0,
+      title: trimmedInputValue,
+      userId: USER_ID,
+      completed: false,
+    });
+    addTodo({ title: trimmedInputValue, userId: USER_ID, completed: false });


Suggested change

setTempTodo({

id: 0,

title: trimmedInputValue,

userId: USER_ID,

completed: false,

});

addTodo({ title: trimmedInputValue, userId: USER_ID, completed: false });

const newTodo = {

title: trimmedInputValue,

userId: USER_ID,

completed: false,

};

setTempTodo({

...newTodo,

id: 0,

});

addTodo(newTodo);

BudnikOleksii · 2023-09-28T10:06:27Z

src/components/TodoItem/TodoItem.tsx

+  const handleUpdateTodo = (val: boolean | string) => {
+    let newTodo = todo;
+
+    if (typeof val === 'boolean') {
+      newTodo = {
+        ...todo,
+        completed: val,
+      };
+    }
+
+    if (typeof val === 'string' && val.trim() !== '') {
+      newTodo = {
+        ...todo,
+        title: val,
+      };
+    }
+
+    if (newTodo) {


Suggested change

const handleUpdateTodo = (val: boolean | string) => {

let newTodo = todo;

if (typeof val === 'boolean') {

newTodo = {

...todo,

completed: val,

};

}

if (typeof val === 'string' && val.trim() !== '') {

newTodo = {

...todo,

title: val,

};

}

if (newTodo) {

const handleUpdateTodo = (updateFields: Partial<Todo>) => {

const updatedTodo = {

...todo,

...updateFields,

};

You can pass directly the fields to update and in this case, you don't need such strange checks. Something like this.

handleUpdateTodo({ completed: !isCompleted });

Also, you always have todo so the last check is redundant.

BudnikOleksii · 2023-09-28T10:09:13Z

src/components/TodoItem/TodoItem.tsx

+    if (!trimmedInputValue) {
+      handleRemoveTodo(todo);
+    }


Suggested change

if (!trimmedInputValue) {

handleRemoveTodo(todo);

}

if (!trimmedInputValue) {

handleRemoveTodo(todo);

return;

}

You have to return after this as in this case, it tries to update todo too and I get an error.

BudnikOleksii · 2023-09-28T10:10:48Z

src/components/TodoItem/TodoItem.tsx

+  useEffect(() => {
+    setIsLoading(changingItems.includes(id));
+  }, [changingItems]);


You don't need isLoading state and this useEffect.
Just const isLoading = changingItems.includes(id); enough

BudnikOleksii · 2023-09-28T10:11:33Z

src/components/TodoList/TodoList.tsx

+type Props = {};
+
+export const TodoList: React.FC<Props> = () => {


Suggested change

type Props = {};

export const TodoList: React.FC<Props> = () => {

export const TodoList = () => {

You don't have any props

BudnikOleksii · 2023-09-28T10:12:02Z

src/components/TodosContext/TodosContext.tsx

+export enum ErrorMessage {
+  Default = '',
+  isLoadTodoError = 'Unable to load todos',
+  isTitleEmpty = 'Title should not be empty',
+  isUnableAddTodo = 'Unable to add a todo',
+  isUnableDeleteTodo = 'Unable to delete a todo',
+  isUnableUpdateTodo = 'Unable to update a todo',
+}


Move this enum to types folder

roman-mirzoian

Good job, but there is quite a bit of work to be done.
Also, now the temporary state of an unupdated todo is saved after closing the editing mode and saves that state for the next such mode:
https://gyazo.com/d88cbd12c346e3f76aabe94f6a736136

roman-mirzoian · 2023-09-29T19:28:26Z

src/components/ErrorNotification/ErrorNotification.tsx

@@ -0,0 +1,45 @@
+/* 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-29T19:28:52Z

src/components/ErrorNotification/ErrorNotification.tsx

+  useEffect(() => {
+    if (timerId.current) {
+      window.clearTimeout(timerId.current);
+    }
+
+    timerId.current = window.setTimeout(() => {
+      setAlarm(ErrorMessage.Default);
+    }, 3000);
+  }, [alarm]);


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

roman-mirzoian · 2023-09-29T19:28:58Z

src/components/ErrorNotification/ErrorNotification.tsx

+    setAlarm,
+  } = useContext(TodosContext);
+
+  const timerId = useRef<number>(0);


Suggested change

const timerId = useRef<number>(0);

const timerIdRef = useRef<number>(0);

roman-mirzoian · 2023-09-29T19:29:11Z

src/components/Footer/Footer.tsx

@@ -0,0 +1,78 @@
+/* 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-29T19:30:15Z

src/components/Footer/Footer.tsx

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


When there is only one todo there should not be plural in the message.

roman-mirzoian · 2023-09-29T19:32:36Z

src/components/Header/Header.tsx

+
+  return (
+    <header className="todoapp__header">
+      { !!todos.length && (


Suggested change

{ !!todos.length && (

{!!todos.length && (

roman-mirzoian · 2023-09-29T19:32:49Z

src/components/TodoItem/TodoItem.tsx

@@ -0,0 +1,173 @@
+/* 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-29T19:33:28Z

src/components/TodoList/TodoList.tsx

@@ -0,0 +1,29 @@
+/* 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-29T19:33:40Z

src/components/TodoList/TodoList.tsx

+          todo={tempTodo}
+        />
+      )}
+


Suggested change

roman-mirzoian · 2023-09-29T19:33:47Z

src/components/TodosContext/TodosContext.tsx

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


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

…ename var, fix plural, rewrite setTimeout into useEffect

roman-mirzoian

Good job, but there is quite a bit of work to be done.
Also, now the temporary state of an unupdated todo is saved after closing the editing mode and saves that state for the next such mode: https://gyazo.com/d88cbd12c346e3f76aabe94f6a736136

Good fixed, but it still doesn't work correct.

roman-mirzoian

Well done 🔥

implement update logic

c7ec635

FrontKid approved these changes Sep 27, 2023

View reviewed changes

remove redundand comments

710ad87

BudnikOleksii suggested changes Sep 28, 2023

View reviewed changes

move vars into separate file, change functions logic

30bc1d3

KatOlista requested a review from BudnikOleksii September 29, 2023 17:05

roman-mirzoian suggested changes Sep 29, 2023

View reviewed changes

rewrite the code so that it is not necessary to disable the linter, r…

fd98591

…ename var, fix plural, rewrite setTimeout into useEffect

KatOlista requested a review from roman-mirzoian September 29, 2023 20:25

roman-mirzoian suggested changes Sep 29, 2023

View reviewed changes

fix uncorrect work of escape event

5e1fe48

KatOlista requested review from roman-mirzoian and FrontKid September 30, 2023 06:26

roman-mirzoian approved these changes Oct 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement update logic, failed from 2 to 5 tests, every time different. Work in progress #806

implement update logic, failed from 2 to 5 tests, every time different. Work in progress #806

KatOlista commented Sep 26, 2023

FrontKid left a comment

BudnikOleksii left a comment

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian Sep 29, 2023

roman-mirzoian left a comment

roman-mirzoian left a comment

		function filterTodos(option: FilterOption, todos: Todo[]) {
		const filteredTodos = todos.filter(todo => {


		const OPTIONS = [FilterOption.All, FilterOption.Completed, FilterOption.Active];

		export const Footer: React.FC<Props> = () => {

	export const Footer: React.FC<Props> = () => {
	export const Footer = () => {

		type Props = {};

		export const TodoList: React.FC<Props> = () => {

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

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

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

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

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

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

implement update logic, failed from 2 to 5 tests, every time different. Work in progress #806

Are you sure you want to change the base?

implement update logic, failed from 2 to 5 tests, every time different. Work in progress #806

Conversation

KatOlista commented Sep 26, 2023

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

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

roman-mirzoian left a comment

Choose a reason for hiding this comment

roman-mirzoian left a comment

Choose a reason for hiding this comment