feat(todo): implement modal opening, filtering, and state management #2752
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements effectively! 🎉 While there are some minor areas for improvement, such as enhancing accessibility with aria-labels and refining some logic for better readability, these do not impact the core functionality. 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! ✨
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!currentTodo || currentTodo.user) { |
There was a problem hiding this comment.
The condition !currentTodo || currentTodo.user might lead to unexpected behavior if currentTodo.user is falsy but valid. Ensure this logic correctly reflects the intended check for whether the user data needs to be fetched.
| setCurrentTodo(prevTodo => { | ||
| if (prevTodo?.id === currentTodo.id) { | ||
| return { ...prevTodo, user }; | ||
| } | ||
|
|
||
| return prevTodo; |
There was a problem hiding this comment.
The logic inside setCurrentTodo could be simplified or clarified. Ensure that the condition prevTodo?.id === currentTodo.id is necessary and correctly handles all cases where the user data should be updated.
| </span> | ||
|
|
||
| <span className="icon is-right" style={{ pointerEvents: 'all' }}> | ||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} |
There was a problem hiding this comment.
The comment indicates that the button lacks an associated label, which can affect accessibility. Consider adding an aria-label or similar attribute to the button to improve accessibility for screen readers.
src/components/TodoList/TodoList.tsx
Outdated
| <tbody> | ||
| {todos.map(todo => { | ||
| return ( | ||
| <tr data-cy="todo" className="" key={todo.id}> |
There was a problem hiding this comment.
Consider adding a meaningful class name to the tr element for better readability and maintainability, especially if additional styling or logic is added in the future.
src/components/TodoList/TodoList.tsx
Outdated
| className={classNames({ | ||
| 'has-text-success': todo.completed, | ||
| 'has-text-danger': !todo.completed, | ||
| })} |
There was a problem hiding this comment.
The use of classNames is correct, but consider extracting the logic into a separate variable for improved readability, especially if more conditions are added later.
| @@ -15,25 +37,36 @@ export const TodoModal: React.FC = () => { | |||
| className="modal-card-title has-text-weight-medium" | |||
| data-cy="modal-header" | |||
| > | |||
| Todo #2 | |||
| Todo #{currentTodo.id} | |||
| </div> | |||
|
|
|||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} | |||
There was a problem hiding this comment.
The comment indicates that the button lacks an associated label, which can affect accessibility. Consider adding an aria-label or similar attribute to the button to improve accessibility for screen readers.
src/App.tsx
Outdated
| .then(data => { | ||
| setTodos(data); | ||
| }) |
There was a problem hiding this comment.
| .then(data => { | |
| setTodos(data); | |
| }) | |
| .then(setTodos) |
src/App.tsx
Outdated
| // // eslint-disable-next-line no-console | ||
| // console.log('Modal state:', isModalOpen, 'ModalLoader:', modalLoading); |
There was a problem hiding this comment.
Remove comments everywhere
| // // eslint-disable-next-line no-console | |
| // console.log('Modal state:', isModalOpen, 'ModalLoader:', modalLoading); |
src/App.tsx
Outdated
| // #endregion State | ||
|
|
||
| // #region Handler |
There was a problem hiding this comment.
| // #endregion State | |
| // #region Handler |
src/App.tsx
Outdated
| if (filterValue === 'completed') { | ||
| return todo.completed === true; | ||
| } else if (filterValue === 'active') { |
There was a problem hiding this comment.
Create a enum for 'all', 'active', 'completed' and use it everywhere. Also, will be better if you use switch case here instead of if conditions
src/App.tsx
Outdated
| return todos | ||
| .filter(todo => { | ||
| if (filterValue === 'completed') { | ||
| return todo.completed === true; |
There was a problem hiding this comment.
| return todo.completed === true; | |
| return todo.completed; |
src/App.tsx
Outdated
| if (filterValue === 'completed') { | ||
| return todo.completed === true; | ||
| } else if (filterValue === 'active') { | ||
| return todo.completed === false; |
There was a problem hiding this comment.
| return todo.completed === false; | |
| return !todo.completed; |
| <option value="all">All</option> | ||
| <option value="active">Active</option> | ||
| <option value="completed">Completed</option> |
src/components/TodoList/TodoList.tsx
Outdated
| {todos.map(todo => { | ||
| return ( |
There was a problem hiding this comment.
| {todos.map(todo => { | |
| return ( | |
| {todos.map(todo => ( |
src/components/TodoList/TodoList.tsx
Outdated
| {todos.map(todo => { | ||
| return ( | ||
| <tr data-cy="todo" className="" key={todo.id}> |
There was a problem hiding this comment.
| {todos.map(todo => { | |
| return ( | |
| <tr data-cy="todo" className="" key={todo.id}> | |
| {todos.map(todo => { | |
| return ( | |
| <tr data-cy="todo" key={todo.id}> |
src/components/TodoList/TodoList.tsx
Outdated
| 'has-text-danger': !todo.completed, | ||
| })} | ||
| > | ||
| {todo.title} |
…mmented code, destructure todo, and improve UI
DEMO LINK