Solution #2713
Solution #2713
Conversation
There was a problem hiding this comment.
Great job on implementing the main components and meeting most of the task requirements! 🎉 While there are some areas for improvement, such as refining event handler naming conventions and simplifying filtering logic, these are minor issues that don't critically impact the functionality. Keep up the good work, and consider the suggested improvements for future projects to enhance code readability and maintainability. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const handlerInput = (e: React.ChangeEvent<HTMLInputElement> | string) => { | ||
| if (typeof e === 'string') { | ||
| setQuery(''); | ||
| } else { | ||
| setQuery(e.target.value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The handlerInput function accepts a parameter that can be either a string or an event. This dual-purpose parameter might lead to confusion. Consider separating the logic or ensuring the function is only called with one type of parameter.
| const handlerSelect = (e: React.ChangeEvent<HTMLSelectElement>) => { | ||
| setShowTodos(e.target.value); | ||
| }; |
There was a problem hiding this comment.
Consider renaming handlerSelect to handleSelectChange for better readability and to follow common naming conventions for event handlers.
| const handlerInput = (e: React.ChangeEvent<HTMLInputElement> | string) => { | ||
| if (typeof e === 'string') { | ||
| setQuery(''); | ||
| } else { | ||
| setQuery(e.target.value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider renaming handlerInput to handleInputChange for better readability and to follow common naming conventions for event handlers.
|
|
||
| type Props = { | ||
| handlerSelect: (event: React.ChangeEvent<HTMLSelectElement>) => void; | ||
| handlerInput: (event: React.ChangeEvent<HTMLInputElement> | string) => void; |
There was a problem hiding this comment.
The handlerInput prop is used for both handling input changes and clearing the input. Consider separating these concerns by using distinct functions for each action to improve clarity.
| handlerSelect: handlerFilter, | ||
| handlerInput: handlerInput, |
There was a problem hiding this comment.
Consider renaming handlerFilter and handlerInput to handleSelectChange and handleInputChange respectively, to follow common naming conventions for event handlers and improve code readability.
| todos: Todo[]; | ||
| select: string; | ||
| query: string; | ||
| handlerModButton: (todo: Todo) => void; |
There was a problem hiding this comment.
Consider renaming handlerModButton to handleModButtonClick to follow common naming conventions for event handlers and improve code readability.
| .filter(todo => { | ||
| if (select === 'all') { | ||
| return todo; | ||
| } | ||
|
|
||
| <tr data-cy="todo" className=""> | ||
| <td className="is-vcentered">1</td> | ||
| <td className="is-vcentered" /> | ||
| <td className="is-vcentered is-expanded"> | ||
| <p className="has-text-danger">delectus aut autem</p> | ||
| </td> | ||
| <td className="has-text-right is-vcentered"> | ||
| <button data-cy="selectButton" className="button" type="button"> | ||
| <span className="icon"> | ||
| <i className="far fa-eye" /> | ||
| </span> | ||
| </button> | ||
| </td> | ||
| </tr> | ||
| return todo.completed === (select === 'completed'); |
There was a problem hiding this comment.
The filtering logic can be simplified. Instead of using an if statement to return todo when select is 'all', you can directly return true for this condition.
| .filter(todo => { | ||
| if (!query) { | ||
| return todo; | ||
| } | ||
|
|
||
| <tr data-cy="todo" className=""> | ||
| <td className="is-vcentered">6</td> | ||
| <td className="is-vcentered" /> | ||
| <td className="is-vcentered is-expanded"> | ||
| <p className="has-text-danger"> | ||
| qui ullam ratione quibusdam voluptatem quia omnis | ||
| </p> | ||
| </td> | ||
| <td className="has-text-right is-vcentered"> | ||
| <button data-cy="selectButton" className="button" type="button"> | ||
| <span className="icon"> | ||
| <i className="far fa-eye" /> | ||
| </span> | ||
| </button> | ||
| </td> | ||
| </tr> | ||
| return todo.title.toLowerCase().includes(query.toLowerCase()); |
There was a problem hiding this comment.
The filtering logic for the query can be simplified. Instead of using an if statement to return todo when query is empty, you can directly return true for this condition.
|
|
||
| type Props = { | ||
| todo: Todo | null; | ||
| handlerModButton: (todo: Todo | null) => void; |
There was a problem hiding this comment.
Consider renaming handlerModButton to handleModButtonClick to follow common naming conventions for event handlers and improve code readability.
| const [loading, setLoading] = React.useState(true); | ||
|
|
||
| useEffect(() => { | ||
| setLoading(true); |
There was a problem hiding this comment.
The loading state is set to true twice: once during initialization and again at the start of the useEffect. The second setting is redundant and can be removed.
DEMO LINK