Develop #2619
Develop #2619
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 The code is well-structured and follows best practices, with only minor areas for improvement. Keep up the good work and consider the feedback for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <TodoFilter | ||
| todos={todosFromServer} | ||
| handleFilterChange={setFilteredTodos} | ||
| /> |
There was a problem hiding this comment.
Ensure that the handleFilterChange function in TodoFilter correctly updates the filteredTodos state. Verify that the TodoFilter component is implemented to handle this function properly, as it is crucial for filtering functionality.
| <TodoList | ||
| todos={filteredTodos} | ||
| selectedTodoId={selectedTodoId} | ||
| handleSelectTodoId={setSelectedTodoId} | ||
| /> |
There was a problem hiding this comment.
Check that the TodoList component correctly handles the handleSelectTodoId function to update the selectedTodoId state. This is important for selecting and displaying the correct todo in the TodoModal.
| <TodoModal | ||
| todo={selectedTodo} | ||
| onSetSelectedTodoId={setSelectedTodoId} |
There was a problem hiding this comment.
Ensure that the TodoModal component uses the onSetSelectedTodoId function to clear or update the selectedTodoId when the modal is closed or the todo is modified. This will help maintain the correct state of the selected todo.
|
|
||
| useEffect(() => { | ||
| handleFilterTodos(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The comment to disable the eslint rule for exhaustive dependencies is present. Ensure that the dependencies for useEffect are correct and that this rule is disabled intentionally. If the handleFilterTodos function changes, consider updating the dependencies accordingly.
| (event: React.ChangeEvent<HTMLSelectElement>) => { | ||
| setFilter(event.target.value as FilterOptions); | ||
| }, | ||
| [], |
There was a problem hiding this comment.
The handleChangeFilter and handleChangeQuery callbacks have empty dependency arrays, which is correct if they don't rely on any external variables. Ensure that this is intentional and that no external state or props are used within these callbacks.
| data-cy="selectButton" | ||
| className="button" | ||
| type="button" | ||
| onClick={() => setSelectedTodoId(todo.id)} |
There was a problem hiding this comment.
Ensure that the setSelectedTodoId function correctly updates the state in the parent component. This is crucial for the correct functioning of the TodoModal display logic.
| isCurrentTodoSelected | ||
| ? 'far fa-eye-slash' | ||
| : 'far fa-eye' |
There was a problem hiding this comment.
The conditional rendering of the icon class names is correct. Ensure that the far fa-eye and far fa-eye-slash classes are correctly styled in your CSS to reflect the intended visual changes when a todo is selected.
| getUser(todo.userId) | ||
| .then(userData => { | ||
| setUser(userData); | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false); | ||
| }); |
There was a problem hiding this comment.
Ensure that the getUser function handles errors gracefully. Consider adding a .catch() block to handle any potential errors during the user data fetch, which would improve the robustness of the component.
| type="button" | ||
| className="delete" | ||
| data-cy="modal-close" | ||
| onClick={() => onSetSelectedTodoId(0)} |
There was a problem hiding this comment.
The onClick handler for the close button correctly resets the selectedTodoId. Ensure that this logic aligns with the parent component's state management to properly close the modal.
| {user ? ( | ||
| <a href={user.email}>{user.name}</a> | ||
| ) : ( | ||
| <span>Loading user...</span> |
There was a problem hiding this comment.
The conditional rendering for the user information is correct. Ensure that the user object has the necessary properties (email and name) and that they are correctly populated from the API response.
https://Vladimir-Zadorozhnyi.github.io/react_dynamic-list-of-todos/