Solution #2589
Solution #2589
Conversation
|
|
||
| let visibleTodos = todos; | ||
|
|
||
| switch (selectedFilter) { |
There was a problem hiding this comment.
better move this switch-case statement to function. The perfect variant will be utils/getFilteredTodos.ts
| <p className="control"> | ||
| <span className="select"> | ||
| <select data-cy="statusSelect" onChange={onSelectChange}> | ||
| <option value="all">All</option> |
There was a problem hiding this comment.
Try to refactor this select for using map method
There was a problem hiding this comment.
Great job on the code! 🎉 While there are minor issues to be improved, they don't impact the functionality or security of the code significantly. Please pay attention to the points raised, they will help you write even better code in the future. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const [selectedFilter, setSelectedFilter] = useState('all'); | ||
| const [query, setQuery] = useState(''); | ||
|
|
||
| const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
The method name 'handleInputChange' does not follow the naming conventions for event handlers in React. Please refer to the link provided in the checklist for the recommended naming conventions.
| setQuery(''); | ||
| }; | ||
|
|
||
| const handleSelectChange = (event: React.ChangeEvent<HTMLSelectElement>) => { |
There was a problem hiding this comment.
The method name 'handleSelectChange' does not follow the naming conventions for event handlers in React. Please refer to the link provided in the checklist for the recommended naming conventions.
| setSelectedFilter(event.target.value); | ||
| }; | ||
|
|
||
| const handleSelectTodo = (todo: Todo) => { |
There was a problem hiding this comment.
The method name 'handleSelectTodo' does not follow the naming conventions for event handlers in React. Please refer to the link provided in the checklist for the recommended naming conventions.
| setSelectedTodo(todo); | ||
| }; | ||
|
|
||
| const handleResetSelectedTodo = () => { |
There was a problem hiding this comment.
The method name 'handleResetSelectedTodo' does not follow the naming conventions for event handlers in React. Please refer to the link provided in the checklist for the recommended naming conventions.
| @@ -17,18 +73,35 @@ | |||
| <h1 className="title">Todos:</h1> | |||
|
|
|||
| <div className="block"> | |||
| <TodoFilter /> | |||
| <TodoFilter | |||
| onSelectChange={handleSelectChange} | |||
There was a problem hiding this comment.
The key 'onSelectChange' does not follow the naming conventions for event handlers in React. Please refer to the link provided in the checklist for the recommended naming conventions.
| data-cy="selectButton" | ||
| className="button" | ||
| type="button" | ||
| onClick={() => onSelectTodo(todo)} |
There was a problem hiding this comment.
This is a minor performance issue. It's generally a good practice to avoid creating new functions inside the render method. Every time this component re-renders, a new function is created in memory. This can be avoided by defining a method on the component that takes the necessary argument, in this case, todo. You could define a method like handleSelectTodo = (todo) => () => { onSelectTodo(todo) } and then use it here like onClick={this.handleSelectTodo(todo)}.
| .finally(() => setLoadingTodoModal(false)); | ||
| }, [selectedTodo.userId]); | ||
|
|
||
| const handleResetUserAndTodo = () => { |
There was a problem hiding this comment.
The name of this function is not clear. According to the react naming conventions, it should start with 'handle' and then describe what it does. For example, you could rename it to 'handleResetUserAndTodo'.
| getUser(selectedTodo.userId) | ||
| .then(setUser) | ||
| .finally(() => setLoadingTodoModal(false)); | ||
| }, [selectedTodo.userId]); |
There was a problem hiding this comment.
You are initializing the state with props, which can lead to bugs and make the component harder to understand. It's better to initialize the state with a default value and then update it based on the props in the useEffect hook.
|
|
||
| export const TodoModal: React.FC = () => { | ||
| return ( |
There was a problem hiding this comment.
You are interacting with the DOM directly here by setting the class name. This is against the react way of doing things. Instead, you should use the classnames library to conditionally set the class name based on the state or props.
| return ( | ||
| <div className="modal is-active" data-cy="modal"> | ||
| <div className="modal-background" /> | ||
|
|
||
| {true ? ( | ||
| {loadingTodoModal ? ( |
There was a problem hiding this comment.
You are generating the key on render here. This can lead to performance issues because react uses the key to identify elements and if the key changes on every render, react will think it's a new element and will re-render it. Instead, you should generate the key when the data is created or received.
|
Good work🔥🔥🔥 |
DEMO LINK