dynamic list of todos #2758

yzhyhaliuk · 2025-02-04T11:11:53Z

https://yzhyhaliuk.github.io/react_dynamic-list-of-todos/

mateacademy-ai-mentor

Thank you for your hard work on this pull request! 😊 While your implementation meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve it:

State Initialization with Props: Initializing state directly from props can lead to unexpected behavior. Please ensure that state is not initialized from props in the TodoList.tsx file.
Dependency Arrays in useEffect: Ensure that dependencies in useEffect hooks are stable to avoid unnecessary re-renders or API calls. This includes stabilizing functions like onFilterActive, onFilterAll, and onFilterCompleted using useCallback.
Handling selectedId: Using 0 as an initial value for selectedId can cause issues if a todo with an ID of 0 exists. Consider using null to indicate no selection, and adjust your logic accordingly.
Conditional Rendering and Classnames: Ensure that elements are conditionally rendered only when data is fully available, and consider using conditional classnames to improve user experience, such as visually indicating the selected todo.

These changes are important to ensure the robustness and clarity of your code. Once these issues are addressed, your pull request will be ready for approval. Keep up the great work! 🌟

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/App.tsx

+  const [allTodos, setAllTodos] = useState<Todo[]>([]);
+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [loading, setLoading] = useState(false);
+  const [selectedId, setSelectedId] = useState(0);


Consider initializing selectedId with null instead of 0 to clearly indicate that no todo is selected initially. This will also help in avoiding potential confusion with a todo having an ID of 0.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/App.tsx

            </div>
          </div>
        </div>
      </div>

-      <TodoModal />
+      {selectedId && (


Using selectedId directly in a boolean context may lead to unexpected behavior if a todo with an ID of 0 exists. Consider checking for selectedId !== 0 or using a more explicit condition.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/components/TodoFilter/TodoFilter.tsx

+  onFilterCompleted,
+  onFilterByTitle,
+}) => {
+  const [isQuery, setIsQuery] = useState(false);


The isQuery state is used to determine if there is a query present. However, since query.length > 0 can be directly checked where needed, you might consider removing isQuery to simplify the state management.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/components/TodoFilter/TodoFilter.tsx

+          break;
+      }
+    }
+  }, [query, filterStatus, onFilterActive, onFilterAll, onFilterCompleted]);


The dependency array for useEffect includes onFilterActive, onFilterAll, and onFilterCompleted. Ensure these functions are stable (e.g., wrapped in useCallback) to prevent unnecessary re-renders.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/components/TodoList/TodoList.tsx

-);
+      <tbody>
+        {todos.map(todo => (
+          <tr data-cy="todo" key={todo.id} className="">


Consider adding a conditional class to the <tr> element to visually indicate the selected todo. This can improve the user experience by clearly showing which todo is currently selected.

mateacademy-ai-mentor · 2025-02-04T11:13:28Z

src/components/TodoList/TodoList.tsx

+                data-cy="selectButton"
+                className="button"
+                type="button"
+                onClick={() => handleSelect(todo.id)}


The handleSelect function is straightforward, but you might consider inlining this logic directly in the onClick handler if it remains simple. This can reduce the number of function declarations and slightly improve readability.

mateacademy-ai-mentor · 2025-02-04T11:13:29Z

src/components/TodoModal/TodoModal.tsx

+        setLoading(false);
+      });
+    }
+  }, [selectedTodo?.userId, selectedTodo]);


The dependency array for useEffect includes selectedTodo?.userId and selectedTodo. Ensure that selectedTodo is stable or derived from a stable source to prevent unnecessary API calls. Consider using id as a dependency instead, since it directly determines the selected todo.

mateacademy-ai-mentor · 2025-02-04T11:13:29Z

src/components/TodoModal/TodoModal.tsx

+              type="button"
+              className="delete"
+              data-cy="modal-close"
+              onClick={() => onClose(0)}


When closing the modal, consider passing null or undefined to onClose instead of 0 to clearly indicate that no todo is selected. This aligns with the suggestion to initialize selectedId with null in the App.tsx file.

mateacademy-ai-mentor · 2025-02-04T11:13:29Z

src/components/TodoModal/TodoModal.tsx


              {' by '}

-              <a href="mailto:[email protected]">Leanne Graham</a>
+              <a href={`mailto: ${data?.email}`}>{data?.name}</a>


Ensure that data?.email and data?.name are available before rendering the link. You might want to conditionally render this part to avoid displaying an incomplete link if the data is not yet loaded.

mateacademy-ai-mentor

Your work on the project has been reviewed, and I'm pleased to say that it meets the requirements! 🎉

You've effectively addressed the previous issues, particularly with the initialization of selectedId and the use of useCallback to stabilize functions. The components are well-structured, and the use of useEffect is appropriate with stable dependencies. The conditional rendering and use of classnames are implemented correctly, enhancing the user interface.

However, there are a few points to keep in mind for future improvements:

Dependency Arrays in useEffect: Ensure that all dependencies in your useEffect hooks are necessary to avoid unnecessary re-renders or re-fetching of data. This is crucial for maintaining performance and ensuring that your application behaves as expected.
Conditional Rendering: Make sure that the logic for rendering components like TodoModal aligns with your application's requirements, especially when dealing with values like 0 or null.
Function Handling: Verify that functions like onSelect and onClose are correctly updating the state in the parent component to maintain the intended flow of data.

Your attention to these details will help you refine your skills and produce even more robust code in the future. Keep up the great work, and continue learning from each project! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2025-02-04T12:29:24Z

src/App.tsx

+  const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]);
+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [loading, setLoading] = useState(false);
+  const [selectedId, setSelectedId] = useState<number | null>(null);


Good job on using null as the initial value for selectedId. This helps avoid potential issues with a todo having an ID of 0.

mateacademy-ai-mentor · 2025-02-04T12:29:24Z

src/App.tsx

+      setTodos(todosFromServer);
+      setLoading(false);
+    });
+  }, []);


The dependency array for useEffect is empty, which means this effect will only run once when the component mounts. Ensure this is the intended behavior, as it will not re-fetch todos if any dependencies change.

mateacademy-ai-mentor · 2025-02-04T12:29:24Z

src/App.tsx

            </div>
          </div>
        </div>
      </div>

-      <TodoModal />
+      {selectedId && (


Ensure that the conditional rendering of TodoModal aligns with your application's logic. Currently, it renders when selectedId is truthy, which means it won't render if selectedId is 0 or null. This is fine if null is intended to represent no selection.

mateacademy-ai-mentor · 2025-02-04T12:29:24Z

src/components/TodoFilter/TodoFilter.tsx

+    onFilterActive,
+    onFilterCompleted,
+    onFilterByTitle,
+  ]);


The useEffect hook has a comprehensive dependency array, which is good for ensuring that the effect runs when any of these dependencies change. Make sure that all dependencies are necessary to avoid unnecessary re-renders. In this case, it seems appropriate as each dependency is related to the filtering logic.

mateacademy-ai-mentor · 2025-02-04T12:29:24Z

src/components/TodoList/TodoList.tsx

+            data-cy="todo"
+            key={todo.id}
+            className={classNames({
+              'has-background-info-light': selectedId === todo.id,


The use of classNames to conditionally apply the has-background-info-light class based on selectedId is correct. This helps visually indicate the selected todo.

mateacademy-ai-mentor · 2025-02-04T12:29:25Z

src/components/TodoList/TodoList.tsx

+                data-cy="selectButton"
+                className="button"
+                type="button"
+                onClick={() => onSelect(todo.id)}


Ensure that the onSelect function is correctly updating the selectedId in the parent component. This line correctly calls onSelect with the todo.id when the button is clicked.

mateacademy-ai-mentor · 2025-02-04T12:29:25Z

src/components/TodoModal/TodoModal.tsx

+        setLoading(false);
+      });
+    }
+  }, [selectedTodo?.userId, selectedTodo]);


The useEffect hook includes selectedTodo?.userId and selectedTodo as dependencies. Ensure that these dependencies are necessary to avoid unnecessary re-fetching of user data. In this case, it seems appropriate as it ensures data is fetched when the selected todo changes.

mateacademy-ai-mentor · 2025-02-04T12:29:25Z

src/components/TodoModal/TodoModal.tsx

+              type="button"
+              className="delete"
+              data-cy="modal-close"
+              onClick={() => onClose(null)}


The onClose function is correctly called with null to reset the selection when the modal is closed. Ensure that this logic is correctly handled in the parent component.

anastasiiavorobiova

Great work! Almost done

anastasiiavorobiova · 2025-02-04T13:12:03Z

src/App.tsx

+    const completedTodos = allTodos.filter(todo => todo.completed);
+
+    setFilteredTodos(completedTodos);
+    setTodos(completedTodos);


It's enough to have one state for filtered todos

I tried, but If I remove filteredTodos, the search through the input field always works on all todos, even if I initially selected "Active" or "Completed". And If I use todos instead of filteredTodos, then when I delete a few letters to modify the search, the list is no longer filtered.

anastasiiavorobiova · 2025-02-04T13:12:32Z

src/components/TodoFilter/TodoFilter.tsx

+  }
+
+  useEffect(() => {
+    if (filterStatus === 'all') {


Consider using an enum for this

volodymyr-soltys97

Good job 👍
Let's improve your code

volodymyr-soltys97 · 2025-02-04T15:17:41Z

src/App.tsx

+  const [allTodos, setAllTodos] = useState<Todo[]>([]);
+  const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]);


Redundant states

Suggested change

const [allTodos, setAllTodos] = useState<Todo[]>([]);

const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]);

volodymyr-soltys97 · 2025-02-04T15:18:40Z

src/App.tsx

+    getTodos().then(todosFromServer => {
+      setAllTodos(todosFromServer);
+      setFilteredTodos(todosFromServer);
+      setTodos(todosFromServer);
+      setLoading(false);
+    });


Suggested change

getTodos().then(todosFromServer => {

setAllTodos(todosFromServer);

setFilteredTodos(todosFromServer);

setTodos(todosFromServer);

setLoading(false);

});

getTodos()

.then(setTodos)

.finally(() => setLoading(false));

volodymyr-soltys97 · 2025-02-04T15:19:08Z

src/components/TodoFilter/TodoFilter.tsx

+enum FilterStatus {
+  All = 'all',
+  Active = 'active',
+  Completed = 'completed',
+}


Move enum to the separate file

volodymyr-soltys97 · 2025-02-04T15:19:36Z

src/components/TodoList/TodoList.tsx

+          <tr
+            data-cy="todo"
+            key={todo.id}
+            className={classNames({
+              'has-background-info-light': selectedId === todo.id,


Move this logic to the PostItem component

volodymyr-soltys97 · 2025-02-04T15:19:49Z

src/components/TodoList/TodoList.tsx

+                  'has-text-danger': !todo.completed,
+                })}
+              >
+                {todo.title}


Use destructuring for todo

volodymyr-soltys97

Great work! 🔥

dynamic list of todos

a433fd7

mateacademy-ai-mentor suggested changes Feb 4, 2025

View reviewed changes

fix tests and errors

1eda377

yzhyhaliuk requested a review from mateacademy-ai-mentor February 4, 2025 12:27

mateacademy-ai-mentor approved these changes Feb 4, 2025

View reviewed changes

anastasiiavorobiova suggested changes Feb 4, 2025

View reviewed changes

create enum

a45c0fe

yzhyhaliuk requested a review from anastasiiavorobiova February 4, 2025 14:03

volodymyr-soltys97 suggested changes Feb 4, 2025

View reviewed changes

rewrite App and filter

c345b58

yzhyhaliuk requested a review from volodymyr-soltys97 February 4, 2025 18:27

volodymyr-soltys97 approved these changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic list of todos #2758

dynamic list of todos #2758

yzhyhaliuk commented Feb 4, 2025 •

edited

Loading

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

mateacademy-ai-mentor Feb 4, 2025

anastasiiavorobiova left a comment

anastasiiavorobiova Feb 4, 2025

yzhyhaliuk Feb 4, 2025

anastasiiavorobiova Feb 4, 2025

volodymyr-soltys97 left a comment

volodymyr-soltys97 Feb 4, 2025

volodymyr-soltys97 Feb 4, 2025

volodymyr-soltys97 Feb 4, 2025

volodymyr-soltys97 Feb 4, 2025

volodymyr-soltys97 Feb 4, 2025

volodymyr-soltys97 left a comment

		const [allTodos, setAllTodos] = useState<Todo[]>([]);
		const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]);

dynamic list of todos #2758

Are you sure you want to change the base?

dynamic list of todos #2758

Conversation

yzhyhaliuk commented Feb 4, 2025 • edited Loading

mateacademy-ai-mentor 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

mateacademy-ai-mentor 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

anastasiiavorobiova 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

volodymyr-soltys97 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

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

yzhyhaliuk commented Feb 4, 2025 •

edited

Loading