Develop #1060

przwojwwp · 2024-10-06T15:22:33Z

DEMO LINK

Zibi95

Hey. Good job. There are some things that has to be fixed. Also i have left some comments to improve the code.

Todo App content div is smaller than its supposed to be. Look how input is overflowing.

The 'Toggle All' button should function by toggling all active to-dos to the completed state. When all to-dos are already completed, it should toggle them back to the active state.

It looks like some tests didn't pass. Did the tests pass locally?

Zibi95 · 2024-10-07T06:49:18Z

src/components/Footer/Footer.tsx

+  const incompleteTodos = todos.filter(todo => todo.completed === false).length;
+  const isEveryIncompleted = todos.every(todo => !todo.completed);


Think about caching results of these calculations.

Zibi95 · 2024-10-07T06:56:07Z

src/components/Footer/Footer.tsx

+export const Footer = () => {
+  const { todos, filter, setFilter, deleteMultipleTodos } = useTodoContext();
+
+  const incompleteTodos = todos.filter(todo => todo.completed === false).length;


This variable name suggests that it holds all the incomplete to-dos themselves, not just the count of them.

Zibi95 · 2024-10-07T06:59:04Z

src/components/Footer/Footer.tsx

+            <a
+              href="#/"
+              className={`filter__link ${filter === 'All' ? 'selected' : ''}`}
+              data-cy="FilterLinkAll"
+              onClick={() => setFilter('All')}
+            >
+              All
+            </a>
+
+            <a
+              href="#/active"
+              className={`filter__link ${filter === 'Active' ? 'selected' : ''}`}
+              data-cy="FilterLinkActive"
+              onClick={() => setFilter('Active')}
+            >
+              Active
+            </a>
+
+            <a
+              href="#/completed"
+              className={`filter__link ${filter === 'Completed' ? 'selected' : ''}`}
+              data-cy="FilterLinkCompleted"
+              onClick={() => setFilter('Completed')}
+            >
+              Completed
+            </a>


There is duplication in setting filters for different views (All, Active, Completed). This can be refactored to reduce redundancy.

Zibi95 · 2024-10-07T07:00:57Z

src/components/Todos/Todos.tsx

+    if (editingTodoId !== null) {
+      if (editingTitle.trim() === '') {
+        deleteTodo(editingTodoId);
+      } else {
+        updateTodoTitle(editingTodoId, editingTitle);
+      }
+    }


The form submission logic could be refactored to avoid deeply nested if conditions.

Suggested change

if (editingTodoId !== null) {

if (editingTitle.trim() === '') {

deleteTodo(editingTodoId);

} else {

updateTodoTitle(editingTodoId, editingTitle);

}

}

if (editingTodoId === null) {

return

}

if (editingTitle.trim() === '') {

deleteTodo(editingTodoId);

} else {

updateTodoTitle(editingTodoId, editingTitle);

}

Zibi95 · 2024-10-07T07:05:54Z

src/components/Todos/Todos.tsx

+    if (headerInputRef.current) {
+      headerInputRef.current.focus();
+    }


Create a utility function that enables triggering focus on headerInputRef.

Zibi95 · 2024-10-07T07:10:56Z

src/components/Todos/Todos.tsx

+  return (
+    <section className="todoapp__main" data-cy="TodoList">
+      {/* This is a completed todo */}
+      {filteredTodos.map(todo => (


Remember about destructure syntax.

Zibi95 · 2024-10-07T07:12:11Z

src/components/Todos/Todos.tsx

+  useEffect(() => {
+    if (editingTodoId !== null && renameInputRef.current) {
+      renameInputRef.current.focus();
+    }
+
+    const handleClickOutsideForm = (event: MouseEvent) => {
+      if (
+        editingTodoId !== null &&
+        renameInputRef.current &&
+        !renameInputRef.current.contains(event.target as Node)
+      ) {
+        if (editingTitle.trim() === '') {
+          deleteTodo(editingTodoId);
+        } else {
+          updateTodoTitle(editingTodoId, editingTitle);
+        }
+
+        setEditingTodoId(null);
+
+        setTimeout(() => {
+          if (headerInputRef.current) {
+            headerInputRef.current.focus();
+          }
+        }, 0);
+      }
+    };


You don't need this logic. You can just simply listen for onBlur event on the input

Zibi95 · 2024-10-07T07:13:53Z

src/components/Todos/Todos.tsx

+    const handlePressEscape = (event: KeyboardEvent) => {
+      if (event.key === 'Escape' && editingTodoId !== null) {
+        setEditingTodoId(null);
+      }
+    };
+
+    if (editingTodoId !== null) {
+      document.addEventListener('mousedown', handleClickOutsideForm);
+      document.addEventListener('keyup', handlePressEscape);
+    }
+
+    return () => {
+      document.removeEventListener('mousedown', handleClickOutsideForm);
+      document.removeEventListener('keyup', handlePressEscape);
+    };
+  });


Add the onKeyUp event to the input. You don't need to listen for the event if it's not necessary.

Zibi95 · 2024-10-07T07:15:37Z

src/components/Todos/Todos.tsx

I suggest creating a TodoItem component. This way, you can colocate the logic and state within that component.

Zibi95 · 2024-10-07T07:18:00Z

src/components/context/TodoContext.tsx

Think about optimizing it a bit. Use useMemo and useCallback.

…text

Zibi95

Good job! There are still some things that you should change.

Zibi95 · 2024-10-09T06:49:00Z

src/components/Footer/Footer.tsx

+                <a
+                  key={key}
+                  href={href}
+                  className={`filter__link ${filter === currentFilter ? 'selected' : ''}`}


Remember that you have installed lib to handle this.

Zibi95 · 2024-10-09T06:51:53Z

src/components/Todos/Todos.tsx

+  useEffect(() => {
+    if (editingTodoId !== null && renameInputRef.current) {
+      renameInputRef.current.focus();
+    }
+  });


It's unnecessary to run on every change. Use a dependency array.

Zibi95 · 2024-10-09T06:56:14Z

src/components/Todos/Todos.tsx

+  const [editingTodoId, setEditingTodoId] = useState<number | null>(null);
+  const [editingTitle, setEditingTitle] = useState('');
+  const renameInputRef = useRef<HTMLInputElement | null>(null);
+  const checkboxRef = useRef<HTMLInputElement | null>(null);
+  const { toggleTodoStatus, updateTodoTitle, deleteTodo, filteredTodos } =
+    useTodoContext();
+
+  const handleSubmit = useCallback(
+    (event: React.FormEvent) => {
+      event.preventDefault();
+
+      if (editingTodoId === null) {
+        return;
+      }
+
+      if (editingTitle.trim() === '') {
+        deleteTodo(editingTodoId);
+      } else {
+        updateTodoTitle(editingTodoId, editingTitle);
+      }
+
+      setEditingTodoId(null);
+    },
+    [deleteTodo, editingTitle, editingTodoId, updateTodoTitle],
+  );
+
+  const handleDoubleClick = useCallback((id: number, currentTitle: string) => {
+    setEditingTodoId(id);
+    setEditingTitle(currentTitle);
+  }, []);
+
+  const handleOnBlur = useCallback(
+    (event: React.FormEvent) => {
+      handleSubmit(event);
+    },
+    [handleSubmit],
+  );
+
+  const handleKeyUp = useCallback(


You've created the TodoItem component, but the logic for editing hasn't been moved there yet. You can move the rename input state and also the editingTodoId. This way, you don't need to track which todo is being edited by its ID; you can simply use a state like isEdited

Zibi95 · 2024-10-09T06:58:37Z

src/components/context/TodoContext.tsx

+  const value = {
+    todos,
+    addTodo,
+    toggleTodoStatus,
+    toggleMultipleTodosStatus,
+    updateTodoTitle,
+    deleteTodo,
+    headerInputRef,
+    filter,
+    setFilter,
+    filteredTodos,
+    deleteMultipleTodos,
+  };


This one also need to be wrapped in memo. This object is recreated on every rerender.

Zibi95 · 2024-10-09T07:04:17Z

src/components/context/TodoContext.tsx

+  const value = {
+    todos,
+    addTodo,
+    toggleTodoStatus,
+    toggleMultipleTodosStatus,
+    updateTodoTitle,
+    deleteTodo,
+    headerInputRef,
+    filter,
+    setFilter,
+    filteredTodos,
+    deleteMultipleTodos,
+  };


What you can do is also separate state and state setters. https://devtrium.com/posts/how-use-react-context-pro#separate-state-and-state-setters-if-necessary

natalia-klonowska

update your demo as your last deployment was 4 days ago and doesn't include all the fixes you've made

przwojwwp · 2024-10-10T12:36:43Z

localy all tests passed

Zibi95

Good job!

przwojwwp added 17 commits October 3, 2024 13:05

feat: introduce add todo logic

8460a5b

feat: add toggle todo status (completed) logic

9103bb2

refactor: extract Header, Todos, Foter to separate components

33d9848

feat: introduce useContext for all components

0928b96

feat: introduce deleteTodo service on button

6e2f350

feat: basic version of todo rename functionality

eafa834

feat: complete todo rename functionality

c50f3d8

feat: update / delete todo title / todo onClick outside form

ef85ef8

refactor: delete useEffect dependency in Todos component

063abc6

feat: save todos to localStorage and load from it

c5ca723

feat: display number of not completed todos

991b156

feat: hide footer if no todos

672db8f

feat: focus headerInput if click outside renaming todo

3ceeb56

feat: add filter change onClick in Footer

310a9a1

feat: delete multiple todos on Clear completed

915d3cb

errored because the property: does not exist on your subject

6a6011c

docs: update demo link path in readme

aa3e0d7

Zibi95 suggested changes Oct 7, 2024

View reviewed changes

przwojwwp added 12 commits October 7, 2024 10:24

feat: set border-box on Header input

c57d9d7

feat: introduce toggleMultipleTodosStatus onClick Header button

a0f2721

feat: save toggleMultipleTodosStatus result in localStorage

94e9dbe

refactor: change incompleteTodos to incompleteTodosCount

a4a9f62

refactor: useEffect instead of hardcoding localStorage

9fdf8a6

refactor: reduce redundancy in Filter

80e1c8d

refactor: avoid conditions deep nesting in Todos

c2e8632

refactor: extract TodoItem from Todos

7fc2b96

refactor: remove inline styles and aplly them with scss

a00ba3f

refactor: destructure todo in Todos and TodoItem

07750b3

refactor: use onBlur and onKeyUp instead of complicated logic

92d6624

feat: playing around with useMemo in Footer

7141800

feat: playing around with useCallback

1b0db7d

przwojwwp requested a review from Zibi95 October 8, 2024 08:08

refactor: extract variables from toggleMultipleTodosStatus in TodoCon…

407a123

…text

Zibi95 suggested changes Oct 9, 2024

View reviewed changes

przwojwwp added 6 commits October 9, 2024 11:32

refactor: add dependency to useEffect which managing focus header input

7e22164

refactor: rename handleKeyUp to handleEscapeUp

4cb58a0

refactor: move simplified management logic to TodoItem

e3510df

feat: useMemo on contextValue

a6c98b9

perf: separate state and state setters in TodoContext

630ebde

feat: toggle todo checkbox if click while renaming

19bc646

przwojwwp requested a review from Zibi95 October 10, 2024 10:47

natalia-klonowska suggested changes Oct 10, 2024

View reviewed changes

chore: deploy actual version of page

6664c98

przwojwwp requested a review from natalia-klonowska October 10, 2024 12:34

Zibi95 approved these changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #1060

Develop #1060

przwojwwp commented Oct 6, 2024

Zibi95 left a comment

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 Oct 7, 2024

Zibi95 left a comment

Zibi95 Oct 9, 2024

przwojwwp Oct 9, 2024

Zibi95 Oct 11, 2024

Zibi95 Oct 9, 2024

Zibi95 Oct 9, 2024

Zibi95 Oct 9, 2024

Zibi95 Oct 9, 2024

natalia-klonowska left a comment

przwojwwp commented Oct 10, 2024

Zibi95 left a comment

		const incompleteTodos = todos.filter(todo => todo.completed === false).length;
		const isEveryIncompleted = todos.every(todo => !todo.completed);

Develop #1060

Are you sure you want to change the base?

Develop #1060

Conversation

przwojwwp commented Oct 6, 2024

Zibi95 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

Zibi95 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

natalia-klonowska left a comment

Choose a reason for hiding this comment

przwojwwp commented Oct 10, 2024

Zibi95 left a comment

Choose a reason for hiding this comment