Add task solution #2606
Add task solution #2606
Conversation
| const [selectedTodo, setSelectedTodo] = useState<Todo | null>(null); | ||
| const [userDetails, setUserDetails] = useState<User | null>(null); | ||
| const [filterStatus, setFilterStatus] = useState('all'); | ||
| const [query, setQuery] = useState(''); |
There was a problem hiding this comment.
Suggestion: states userDetails and isLoadingUser are needed only in TodoModal. You can create useEffect for them as you do for todos
src/components/TodoList/TodoList.tsx
Outdated
| <tr | ||
| key={todo.id} | ||
| data-cy="todo" | ||
| className={todo.completed ? 'has-background-info-light' : ''} |
There was a problem hiding this comment.
This is causing a problem. You don't need to highlight completed todos. You should highlight when it is your selectedTodo. But in your case it SelectedTodoId
There was a problem hiding this comment.
Great job on this pull request! Your code is clean and well-structured. There are a few minor issues, but they don't significantly impact the functionality of your code. Keep up the good work! 🎉 Remember to consider the provided feedback for your future tasks. Happy coding! 😄
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const handleSelectTodo = (todoId: number | null) => { | ||
| setSelectedTodoId(todoId); | ||
| }; |
There was a problem hiding this comment.
The naming convention for the method is not followed. It should be 'onSelectTodo' instead of 'handleSelectTodo'. Please refer to the checklist link for naming conventions.
| const closeModal = () => { | ||
| setSelectedTodoId(null); | ||
| }; |
There was a problem hiding this comment.
The naming convention for the method is not followed. It should be 'onCloseModal' instead of 'closeModal'. Please refer to the checklist link for naming conventions.
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); | ||
| const [isLoadingTodos, setIsLoadingTodos] = useState(false); | ||
| const [selectedTodoId, setSelectedTodoId] = useState<number | null>(null); | ||
| const [filterStatus, setFilterStatus] = useState('all'); | ||
| const [query, setQuery] = useState(''); |
There was a problem hiding this comment.
You are initializing your state with props. This can lead to bugs since the state will not update when the props change. Always use props directly or make sure to update the state when the props change.
| className={ | ||
| todo.id === selectedTodo ? 'has-background-info-light' : '' | ||
| } |
There was a problem hiding this comment.
Consider using the classnames library to conditionally apply classes. It allows for cleaner and more readable code. You can find the library here.
| data-cy="selectButton" | ||
| className="button" | ||
| type="button" | ||
| onClick={() => setSelectedTodo(null)} |
There was a problem hiding this comment.
The naming convention for event handlers could be improved. Instead of setSelectedTodo, consider using handleSelectedTodo or onSelectTodo. This makes it clear that this function is an event handler. You can find more information on naming conventions here.
| data-cy="selectButton" | ||
| className="button" | ||
| type="button" | ||
| onClick={() => setSelectedTodo(todo.id)} |
There was a problem hiding this comment.
The same naming convention issue is present here. Consider using handleSelectedTodo or onSelectTodo instead of setSelectedTodo.
|
|
||
| export const TodoModal: React.FC<Props> = ({ todo, closeModal }) => { | ||
| const [user, setUser] = useState<User | null>(null); | ||
| const [isLoadingUser, setIsLoadingUser] = useState(true); |
There was a problem hiding this comment.
It seems that you are initializing the state with props (todo.userId in this case). This is generally considered an anti-pattern in React. This is because the constructor or useState hook runs only once when the component is created, and then never runs again. This means that even if the prop updates, the state will not update. You should avoid copying props into state and instead use the prop directly.
| useEffect(() => { | ||
| setIsLoadingUser(true); | ||
| getUser(todo.userId) | ||
| .then(userData => setUser(userData)) | ||
| .finally(() => setIsLoadingUser(false)); | ||
| }, [todo.userId]); |
There was a problem hiding this comment.
You are directly interacting with the API inside the component. This could lead to performance issues and make the component less reusable. Consider moving the API calls outside of the component, for instance using custom hooks. This way, you could separate the data-fetching logic from the component, making your code cleaner and easier to test.
https://WatarJoy.github.io/react_dynamic-list-of-todos/