-solution #2710
-solution #2710
Conversation
There was a problem hiding this comment.
Great job on implementing the todo list application! 🎉 The code is well-structured and meets the core requirements of the task. There are a few minor issues related to the handling of filter values and the select state that you might want to address for improved functionality and consistency. Keep up the good work and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| function filterTodos(list: Todo[], params: Params) { | ||
| const { query, filter } = params; | ||
| let result: Todo[] = []; | ||
|
|
||
| if (filter) { | ||
| switch (filter) { | ||
| case 'all': | ||
| result = list; | ||
| break; | ||
|
|
||
| case 'active': | ||
| result = list.filter((todo: Todo) => !todo.completed); | ||
| break; | ||
|
|
||
| case 'completed': | ||
| result = list.filter((todo: Todo) => todo.completed); | ||
| break; | ||
|
|
||
| default: | ||
| result = list; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (query) { | ||
| result = result.filter((todo: Todo) => | ||
| todo.title.toLowerCase().includes(query.toLowerCase()), | ||
| ); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The filterTodos function uses a filter parameter, but the switch statement does not handle the case where filter is 'All'. This could lead to unexpected behavior since the initial state for filter is set to 'All'. Consider adding a case for 'All' or adjusting the initial state to match one of the handled cases.
|
|
||
| const [select, setSelect] = useState(0); | ||
|
|
||
| const [filter, setFilter] = useState('All'); |
There was a problem hiding this comment.
The initial state for filter is set to 'All', but this value is not handled in the filterTodos function. Ensure that the initial state matches one of the cases handled in the filterTodos function or add a case for 'All'.
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [loading, setLoading] = useState(false); | ||
|
|
||
| const [select, setSelect] = useState(0); |
There was a problem hiding this comment.
The select state is initialized with 0. Ensure that this initial value is intentional and aligns with the logic in your application, especially in the context of the TodoModal component.
| data-cy="statusSelect" | ||
| onChange={event => setFilter(event.target.value)} | ||
| > | ||
| <option value="all">All</option> |
There was a problem hiding this comment.
The option value for 'All' is set to 'all'. Ensure that this matches the expected filter values in the filterTodos function in App.tsx. Currently, the filterTodos function does not handle 'all', which could lead to unexpected behavior.
| {todos.map(todo => ( | ||
| <tr | ||
| data-cy="todo" | ||
| className={cn(select === todo.id && 'has-background-info-light')} |
There was a problem hiding this comment.
The className conditionally applies 'has-background-info-light' when select equals todo.id. Ensure that the select prop is correctly managed and updated to reflect the intended selection logic. If select is initialized with 0, verify that this aligns with your application's logic for selecting todos.
| return ( | ||
| <div className="modal is-active" data-cy="modal"> | ||
| <div className="modal-background" /> | ||
| {select !== 0 && <div className="modal-background" />} |
There was a problem hiding this comment.
The modal background is displayed when select is not 0. Ensure that this logic aligns with the intended behavior of your application. If select is initialized with 0, verify that this condition is appropriate for showing the modal background.
| </div> | ||
| todos.map( | ||
| todo => | ||
| select === todo.id && ( |
There was a problem hiding this comment.
The condition select === todo.id is used to render the modal content. Ensure that the select state is correctly managed to reflect the intended selection logic and avoid displaying incorrect data.
DEMO LINK