initial commit #794

cardinal1991 · 2023-09-25T22:19:06Z

DEMO LINK

choeqq

PLEASE MAKE SURE TO REMOVE EVERY COMMENT BEFORE PUSHING YOUR CODE

also we want to use comments only we unexpected behaviour occurs 😉

choeqq · 2023-09-27T06:45:35Z

src/components/TodoContext.tsx

I think this file should be in different directory

choeqq · 2023-09-27T06:45:52Z

src/components/TodoContext.tsx

+  // const addTodo = (title: string) => {
+  //   // Dodaj zadanie do stanu zadań
+  //   setTodos([...todos: any, { title, completed: false }]);
+  // };


remove comments

choeqq · 2023-09-27T06:47:24Z

src/components/TodoContext.tsx

you need to type your context corretly, then you won't have to type it manually every time you use it

choeqq · 2023-09-27T06:47:40Z

src/App.tsx

+    // isToggledAll,
+    setIsToggledAll,
+    // isGroupDeleting,


choeqq · 2023-09-27T06:47:54Z

src/App.tsx

+    // isGroupDeleting,
+    setIsGroupDeleting,
+    titleInputRef,
+  } = useTodoContext() as TContext;


check comments in TodoContext.tsx

choeqq · 2023-09-27T06:50:49Z

src/components/TodoItem.tsx

+  // const handleDelete = (todoId: number) => {
+  //   setIsDeleting(true);
+
+  //   return deleteTodo(todoId)
+  //     .then(() => getTodos(USER_ID))
+  //     .then((res) => {
+  //       setTodos(res);
+  //       setIsDeleting(false);
+  //     })
+  //     .catch(() => {
+  //       handleError('Unable to delete a todo');
+  //     })
+  //     .finally(() => {
+  //       titleInputRef.current?.focus();
+  //     });
+  // };


choeqq · 2023-09-27T06:51:03Z

src/components/TodoList.tsx

@@ -0,0 +1,75 @@
+import React from 'react';
+// import cn from 'classnames';


choeqq · 2023-09-27T06:51:09Z

src/components/TodoList.tsx

+    // todos,
+    // setTodos,
+    // handleError,
+    tempTodos,
+    // idTemp,


choeqq · 2023-09-27T06:51:13Z

src/components/TodoList.tsx

+    // handleError,
+    tempTodos,
+    // idTemp,
+  } = useTodoContext() as TContext;


check comments in TodoContext.tsx

choeqq · 2023-09-27T06:51:27Z

src/components/TodoList.tsx

+      {/* {(tempTodos !== null) && (
+        <div data-cy="Todo" className={`${tempTodos?.completed ? 'todo completed' : 'todo'}`} key={tempTodos?.id}>
+          <label className="todo__status-label">
+            <input
+              data-cy="TodoStatus"
+              type="checkbox"
+              className="todo__status"
+              checked={tempTodos?.completed}
+            />
+          </label>
+
+          <span data-cy="TodoTitle" className="todo__title">
+            {tempTodos?.title}
+          </span>
+          <button
+            type="button"
+            className="todo__remove"
+            data-cy="TodoDelete"
+          >
+            ×
+          </button>
+
+          <div
+            data-cy="TodoLoader"
+            className={cn('modal overlay')}
+            // { 'is-active': tempTodos.id === 0 }
+
+          >


Matylda-Lewandowska

I have nothing to add, code looks nice to me :)

pawel-peksa

Almost there ;)

pawel-peksa · 2023-09-28T06:58:56Z

src/index.tsx

@@ -1,10 +1,16 @@
 import { createRoot } from 'react-dom/client';
+// import React from 'react';


Cleanup needed

pawel-peksa · 2023-09-28T06:59:41Z

src/context/TodoContext.tsx

+      // eslint-disable-next-line max-len
+      const updatedTodoWithNewStatus = { ...updatedTodo, completed: !updatedTodo.completed };


Please use proper formatting and remove // eslint-disable-next-line max-len

pawel-peksa · 2023-09-28T07:03:30Z

src/context/TodoContext.tsx

+
+export function TodoProvider({ children }: { children: ReactNode }) {
+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [hasError, setHasError] = useState<string | null>(null);


Let's narrow down the type of this state to union of strings which we are actually using there + null

pawel-peksa · 2023-09-28T07:05:30Z

src/context/TodoContext.tsx

+  todos: Todo[];
+  setTodos: React.Dispatch<SetStateAction<Todo[]>>;
+  hasError: null | string;
+  handleError: (error: string) => void;


Please see the comment related to types in const [hasError, setHasError].
Instead of string let's use a string union

pawel-peksa · 2023-09-28T07:07:00Z

src/components/TodoItem.tsx

+              className="todo__title-field"
+              placeholder="Empty todo will be deleted"
+              value={newTitle}
+              // defaultValue={todo.title}


Cleanup needed

pawel-peksa · 2023-09-28T07:18:58Z

src/App.tsx

+    // }
+  };
+
+  const arrayCompleted = [...todos].filter((todo) => todo.completed === true);


Suggested change

const arrayCompleted = [...todos].filter((todo) => todo.completed === true);

const arrayCompleted = [...todos].filter((todo) => todo.completed);

pawel-peksa · 2023-09-28T07:19:55Z

src/App.tsx

  if (!USER_ID) {
    return <UserWarning />;
  }

+  const isAllCompleted = todos.every((todo) => todo.completed === true);


Suggested change

const isAllCompleted = todos.every((todo) => todo.completed === true);

const isAllCompleted = todos.every((todo) => todo.completed);

pawel-peksa · 2023-09-28T07:21:17Z

src/App.tsx

+    // }
+  };
+
+  const arrayCompleted = [...todos].filter((todo) => todo.completed === true);


Same logic as in sortedTodos.completed. Try to not repeat your code.

pawel-peksa · 2023-09-28T07:22:32Z

src/App.tsx

+                // eslint-disable-next-line quote-props
+                  'active': (isAllCompleted),


No need for quotes here

pawel-peksa · 2023-09-28T07:23:29Z

src/App.tsx

+              className="todoapp__clear-completed"
+              data-cy="ClearCompletedButton"
+              onClick={deleteCompleted}
+              disabled={!(todos.some(todo => todo.completed === true))}


Suggested change

disabled={!(todos.some(todo => todo.completed === true))}

disabled={!(todos.some(todo => todo.completed))}

pawel-peksa

Great job 🔥 This task is really complicated and the code looks much better. There is, however, still a possiblity to fix many of the failing tests. Try to focus on setting 'is-active' class basing on specific states. Currently it looks like there is too much going on. First, try to write down on paper all the cases when this class needs to be applied and then reflect it in isActiveConditions

pawel-peksa · 2023-09-28T09:45:32Z

src/App.tsx

+    }
+  };
+
+  // const arrayCompleted = [...todos].filter((todo) => todo.completed);


cleanup needed

pawel-peksa · 2023-09-28T09:47:10Z

src/components/TodoItem.tsx

+  const isActiveConditions = (isDeleting === true)
+  || (todo.id === 0) || (isToggled && todo.id === toggledId)
+  || (isToggledAll) || (isLoading)
+  || ((todo.completed) && isGroupDeleting);
+


It's good that you extracted it outside JSX, however this logic still causes some of the tests to fail. Try to fix it a bit. It should not be dependent on so many cases.

pawel-peksa · 2023-09-28T09:51:00Z

src/App.tsx

+              type="button"
+              className={cn('todoapp__toggle-all',
+                {
+                  active: (isAllCompleted || isAllNotCompleted),


I think you want this class to appear only when isAllCompleted is true.

mbruhler

Good job, you are almost there!

Just a few fixes in your API handling.
Remember that:

In PATCH request, you don't have to (and you shouldn't) pass whole object to update, just partial data that needs the update
Always read the backend API documentation and align your API handling to it. For example, you don't have to pass ID to the POST request. If there are typing issues, think of using Partial, Pick, Omit typescript utility types which helps to have one type and it's transformations.

In your case I would go with creating types like that:

Todo - which contains the response of Todo from API
PatchTodo - which is a Partial type of Todo - Partial<Todo>. This makes all the properties optional,
AddTodo - which has everything but id. So you can use Omit here like Omit<Todo, 'id'>
others accordingly if needed

More on this here: https://www.typescriptlang.org/docs/handbook/utility-types.html

Of course you don't have to use those directives, you can create just another types.

My suggestion may be an overkill for now, but I can see that you do really good and you have much potential so why not trying something extra! If that's over the top, next person propably will accept this PR :)

mbruhler · 2023-09-29T06:58:55Z

src/components/TodoForm.tsx

+    titleInputRef,
+  } = useTodoContext() as TContext;
+
+  let counter: number;


This one resolves to NaN

mbruhler · 2023-09-29T06:59:38Z

src/components/TodoForm.tsx

+  const handleSubmit = (event: FormEvent<HTMLFormElement>) => {
+    event.preventDefault();
+
+    const newTodo: Todo = {


{ "userId": 11550, "id": NaN, "title": "a", "completed": false }

mbruhler · 2023-09-29T07:00:23Z

src/components/TodoForm.tsx

+        .finally(() => {
+          setIsSubmitting(false);
+          setTempTodos(null);
+          counter += 1;


In overall this counter has no effect because is always null. If you want to get next ID then you should check it against the last item in the todos collection.

Actually as I can see you don't have to pass ID to the POST

Please refer to this documentation link: https://mate-academy.github.io/fe-students-api/todos.html

mbruhler · 2023-09-29T07:10:25Z

src/context/TodoContext.tsx

+
+      setIsToggled(true);
+
+      editTodo(todoId, updatedTodoWithNewStatus)


You don't have to pass the whole Todo to the PATCH.
It's always good to transfer at least amount of data between backend and frontend.

Here we are only interested to set the completed status to the opposite, so the payload should be

{ completed: !updatedTodo.completed }

Please refer to the API documentation for further information: https://mate-academy.github.io/fe-students-api/todos.html

…leDelete moved to Context and editTodo with partial argument

mbruhler

Cleanup the code and I'll accept it. Check also if there are no not needed console.log and // comments

mbruhler · 2023-09-29T12:44:34Z

src/components/TodoForm.tsx

@@ -43,6 +40,8 @@ export const TodoForm: React.FC = () => {
      addTodo(newTodo)
        .then((res) => {
          setTodos([...todos, res]);
+          // eslint-disable-next-line no-console


clear console.log

mbruhler · 2023-09-29T12:45:27Z

src/App.tsx

@@ -7,23 +7,21 @@ import { TodoList } from './components/TodoList';
 import { TodoForm } from './components/TodoForm';
 import { TContext, useTodoContext } from './context/TodoContext';
 import { SortTypes, Todo } from './types/Todo';
-import { deleteTodo } from './api/todos';
+// import { deleteTodo } from './api/todos';


clear comment

choeqq

LGTM ✅

cardinal1991 added 7 commits September 26, 2023 00:16

initial commit

df898bc

added ref method in context to focus input after deleting, toggling etc.

6060af2

added deleteCompleted method and isGroupDeleting state

6066752

testing test speed according to the deleting item

65d6f20

testing deleting locally before deleting on server Completet Group

c42faaa

disabled Clear completed on condition

dce3933

testing focus on doubleclick in useEffect

f263fd8

choeqq suggested changes Sep 27, 2023

View reviewed changes

cardinal1991 added 9 commits September 27, 2023 12:14

sol deleted comments

070cbac

sol SortTypes adjustment in TodoFilter

cf0bad5

sol TodoContext moved to context folder

6c0fa13

sol togglingAll disabled on empty list

1f1beaf

sol for testing togglingAll if allisCompleted

b40add3

sol for testing togglingAll if allisCompletedp2

ebc5a7b

sol Esc event leaving old title

28309ca

sol just testing deleteCompleted

fb2fcfa

testing prev ver second time

9c369f6

cardinal1991 requested a review from choeqq September 27, 2023 14:44

Matylda-Lewandowska approved these changes Sep 27, 2023

View reviewed changes

pawel-peksa suggested changes Sep 28, 2023

View reviewed changes

sol added ErrorTypes, code refactor, cleaning

9917ed4

cardinal1991 requested a review from pawel-peksa September 28, 2023 08:45

pawel-peksa suggested changes Sep 28, 2023

View reviewed changes

sol removed redundant getTodos

809f786

cardinal1991 requested a review from pawel-peksa September 28, 2023 12:12

mbruhler suggested changes Sep 29, 2023

View reviewed changes

sol added types TodoAdd(Omit<Todo, id>) TodoEdit(Partial<Todo>), hand…

4f5ff13

…leDelete moved to Context and editTodo with partial argument

cardinal1991 requested a review from mbruhler September 29, 2023 11:32

mbruhler suggested changes Sep 29, 2023

View reviewed changes

sol cleaning comments and unnecessary consol.logs

abcd368

cardinal1991 requested a review from mbruhler September 29, 2023 20:58

choeqq approved these changes Oct 2, 2023

View reviewed changes

		@@ -0,0 +1,75 @@
		import React from 'react';
		// import cn from 'classnames';

		@@ -1,10 +1,16 @@
		import { createRoot } from 'react-dom/client';
		// import React from 'react';

		// eslint-disable-next-line max-len
		const updatedTodoWithNewStatus = { ...updatedTodo, completed: !updatedTodo.completed };

	const arrayCompleted = [...todos].filter((todo) => todo.completed === true);
	const arrayCompleted = [...todos].filter((todo) => todo.completed);

	const isAllCompleted = todos.every((todo) => todo.completed === true);
	const isAllCompleted = todos.every((todo) => todo.completed);

		// eslint-disable-next-line quote-props
		'active': (isAllCompleted),

	disabled={!(todos.some(todo => todo.completed === true))}
	disabled={!(todos.some(todo => todo.completed))}


		setIsToggled(true);

		editTodo(todoId, updatedTodoWithNewStatus)

initial commit #794

Are you sure you want to change the base?

initial commit #794

Conversation

cardinal1991 commented Sep 25, 2023

choeqq 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

Matylda-Lewandowska left a comment

Choose a reason for hiding this comment

pawel-peksa 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

pawel-peksa 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

mbruhler 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

mbruhler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choeqq left a comment

Choose a reason for hiding this comment