Develop #789

Koliras · 2023-09-25T09:07:54Z

Vannnkof

Good job!
Found just one bug

roman-mirzoian

Good job, but there is quite a bit of work to be done.
Also, pay attention to the previous comment about the visual component of the application.

roman-mirzoian · 2023-09-26T19:57:21Z

src/components/NewTodo/NewTodo.tsx

+        className="todoapp__new-todo"
+        placeholder="What needs to be done?"
+        value={newTodoTitle}
+        onChange={(event) => setNewTodoTitle(event.target.value)}


Suggested change

onChange={(event) => setNewTodoTitle(event.target.value)}

onChange={({ target }) => setNewTodoTitle(target.value)}

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

src/components/TodoApp/TodoApp.tsx

+  const [filterKey, setFilterKey] = useState<FilterKey>(FilterKey.All);
+  const [tempTodo, setTempTodo] = useState<Todo | null>(null);
+
+  const areAllTodosCompleted = todos.every(({ completed }) => completed);


I know that "are" would be lexically correct, but in programming it is customary to use "is" in all cases :)

roman-mirzoian · 2023-09-26T20:00:36Z

src/components/TodoApp/TodoApp.tsx

+        {!!todos.length && (
+          <footer className="todoapp__footer" data-cy="Footer">
+            <span className="todo-count" data-cy="TodosCounter">
+              {`${activeTodos.length} items left`}


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

Firstly, I wrote a code that would not use plural to a single todo, but tests required it to be plural

roman-mirzoian · 2023-09-26T20:01:02Z

src/components/TodoError/TodoError.tsx

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


Suggested change

classNames('notification is-danger is-light has-text-weight-normal', {

classNames('notification', 'is-danger', 'is-light', 'has-text-weight-normal', {

roman-mirzoian · 2023-09-26T20:01:24Z

src/components/TodoItem/TodoItem.tsx

+  const { title, completed, id } = todo;
+  const [isBeingEdited, setIsBeingEdited] = useState(false);
+  const [newTitle, setNewTitle] = useState(title);
+  const editedInput = useRef<HTMLInputElement | null>(null);


Suggested change

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

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

roman-mirzoian · 2023-09-26T20:02:10Z

src/components/TodoItem/TodoItem.tsx

+              ref={editedInput}
+              onChange={handleChangeOfTitle}
+              onKeyUp={handleKeyUp}
+              onBlur={() => handleNewTitleSubmit()}


Suggested change

onBlur={() => handleNewTitleSubmit()}

onBlur={handleNewTitleSubmit}

roman-mirzoian · 2023-09-26T20:03:08Z

src/components/TodoItem/TodoItem.tsx

+        className={classNames('modal overlay', {
+          'is-active': todo.id === 0 || todoIdsWithLoader.includes(id),
+        })}


Suggested change

className={classNames('modal overlay', {

'is-active': todo.id === 0 || todoIdsWithLoader.includes(id),

})}

className={classNames('modal', 'overlay', {

'is-active': !todo.id || todoIdsWithLoader.includes(id),

})}

roman-mirzoian · 2023-09-26T20:04:07Z

src/components/NewTodo/NewTodo.tsx

+    event.preventDefault();
+
+    if (!newTodoTitle.trim()) {
+      onNewError('Title should not be empty');


It would be good to put error texts in a separate constant.

Moroz-Dmytro · 2023-09-27T11:12:52Z

src/components/TodoApp/TodoApp.tsx

+    todosToUpdate.forEach(todo => {
+      updateTodo(todo.id, { completed: !todo.completed })
+        .then(() => {
+          const todosCopy = [...todos];
+          const searchedTodo = todos.find(
+            ({ id }) => todo.id === id,
+          ) as Todo;
+
+          searchedTodo.completed = !searchedTodo.completed;
+
+          setTodos(todosCopy);
+        })
+        .catch(() => onNewError(ErrorMessage.UnableUpdate))
+        .finally(() => setTodoIdsWithLoader(
+          prevTodoIds => prevTodoIds.filter((todoId) => todoId !== todo.id),
+        ));
+    });


Let's use Promise.all for updating todos

Moroz-Dmytro · 2023-09-27T11:14:27Z

src/components/TodoApp/TodoApp.tsx

+      deleteTodo(todo.id)
+        .then(() => {
+          setTodos(prevTodos => prevTodos.filter(({ id }) => id !== todo.id));
+        })
+        .catch(() => onNewError(ErrorMessage.UnableDelete))
+        .finally(() => setTodoIdsWithLoader(
+          prevTodoIds => prevTodoIds.filter((id) => todo.id !== id),
+        ));


Also, try to use Promise.all

Moroz-Dmytro · 2023-09-27T11:18:10Z

src/components/TodoItem/TodoItem.tsx

+      <label
+        className="todo__status-label"
+      >


Suggested change

<label

className="todo__status-label"

>

<label className="todo__status-label">

Moroz-Dmytro · 2023-09-27T11:19:29Z

src/components/TodoApp/TodoApp.tsx

+import { ErrorContext } from '../ErrorContextProvider/ErrorContextProvider';
+import { ErrorMessage } from '../../types/ErrorMessage';
+
+function getFilteredTodos(key: FilterKey, todos: Todo[]) {


I would move this to separate file as a helper function

Moroz-Dmytro · 2023-09-27T11:21:17Z

src/components/TodoApp/TodoApp.tsx

+        />
+
+        {!!todos.length && (
+          <footer className="todoapp__footer" data-cy="Footer">


I would move footer to separate component

BudnikOleksii

Good work 🚀
Approved, but left a few suggestions, take a look.

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

src/api/todos.ts

+  return client.delete(`/todos/${todoId}`);
+};
+
+export const updateTodo = (todoId: number, changes: object) => {


changes have to have another type instead of an object, as we update a todo in this case, and not every object suits.
You can use some of the utility types, for example, Partial.
https://www.typescriptlang.org/docs/handbook/utility-types.html

BudnikOleksii · 2023-09-28T09:26:06Z

src/components/ErrorContextProvider/ErrorContextProvider.tsx

+export const ErrorContext = React.createContext({
+  errorMessage: ErrorMessage.None,
+  setErrorMessage: () => {},
+  onNewError: () => {},
+} as ErrorContextProps);


Suggested change

export const ErrorContext = React.createContext({

errorMessage: ErrorMessage.None,

setErrorMessage: () => {},

onNewError: () => {},

} as ErrorContextProps);

export const ErrorContext = React.createContext<ErrorContextProps>({

errorMessage: ErrorMessage.None,

setErrorMessage: () => {},

onNewError: () => {},

});

Don't use as, createContext accepts generic

BudnikOleksii · 2023-09-28T09:30:36Z

src/components/TodoApp/TodoApp.tsx

+        <header className="todoapp__header">
+          {!!todos.length && (
+            <button
+              type="button"
+              data-cy="ToggleAllButton"
+              className={classNames('todoapp__toggle-all', {
+                active: isAllTodosCompleted,
+              })}
+              aria-label="toggle_all_todos"
+              onClick={handleAllTodosToggle}
+            />
+          )}
+
+          <NewTodo
+            setTempTodo={setTempTodo}
+            tempTodo={tempTodo}
+          />
+        </header>


You can create a Header component

BudnikOleksii · 2023-09-28T09:33:47Z

src/components/TodoItem/TodoItem.tsx

+        const todosCopy = [...todos];
+        const searchedTodo = todos.find(
+          ({ id: todoId }) => todoId === id,
+        ) as Todo;
+
+        searchedTodo.completed = !event.target.checked;
+
+        setTodos(todosCopy);


Suggested change

const todosCopy = [...todos];

const searchedTodo = todos.find(

({ id: todoId }) => todoId === id,

) as Todo;

searchedTodo.completed = !event.target.checked;

setTodos(todosCopy);

setTodos(prevTodos => (

prevTodos.map(todo => todoId === todo.id ? updatedTodo : todo)

));

Use map instead of find.

Koliras added 3 commits September 25, 2023 11:52

implement toggling and editing todos, fixed deleting completed todos

1091178

delete unnecessary comments, improve some functions

ddb1cc6

add readme file

4b7d941

Vannnkof approved these changes Sep 26, 2023

View reviewed changes

roman-mirzoian suggested changes Sep 26, 2023

View reviewed changes

add const for errors, fix visual bug, change error message showing

c6ed7e7

Koliras requested a review from roman-mirzoian September 27, 2023 08:46

Moroz-Dmytro suggested changes Sep 27, 2023

View reviewed changes

make Footer component, replace foreach with Promise.all

4f8d5fd

Koliras requested a review from Moroz-Dmytro September 27, 2023 13:44

BudnikOleksii approved these changes Sep 28, 2023

View reviewed changes

Update README.md

affb0aa

Koliras closed this by deleting the head repository Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #789

Develop #789

Koliras commented Sep 25, 2023

Vannnkof left a comment

roman-mirzoian left a comment

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

Koliras Sep 27, 2023 •

edited

Loading

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

Moroz-Dmytro Sep 27, 2023

Moroz-Dmytro Sep 27, 2023

Moroz-Dmytro Sep 27, 2023

Moroz-Dmytro Sep 27, 2023

Moroz-Dmytro Sep 27, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

BudnikOleksii Sep 28, 2023

	onChange={(event) => setNewTodoTitle(event.target.value)}
	onChange={({ target }) => setNewTodoTitle(target.value)}

	classNames('notification is-danger is-light has-text-weight-normal', {
	classNames('notification', 'is-danger', 'is-light', 'has-text-weight-normal', {

	const editedInput = useRef<HTMLInputElement \| null>(null);
	const editedInputRef = useRef<HTMLInputElement \| null>(null);

	onBlur={() => handleNewTitleSubmit()}
	onBlur={handleNewTitleSubmit}

Develop #789

Develop #789

Conversation

Koliras commented Sep 25, 2023

Vannnkof 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

Koliras Sep 27, 2023 • edited Loading

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

Koliras Sep 27, 2023 •

edited

Loading