solution #1558
solution #1558
Conversation
There was a problem hiding this comment.
Good work 👍
Left a few comments. And you have a bug when you change the todo title to an empty string it deletes it but shows an error or sometimes it doesn't delete todo
https://www.loom.com/share/5a5cc06e2f6248d7b2a43d65d3eac3bc?sid=47b640b2-e2bc-4328-9cb4-2b1ba5317c83
src/App.tsx
Outdated
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [activeStatus, setActiveStatus] = useState(Status.All); | ||
| const [tempTodo, setTempTodo] = useState<Todo | null>(null); | ||
| const [loadingTodos, setLoadingTodos] = useState<number[]>([]); |
There was a problem hiding this comment.
| const [loadingTodos, setLoadingTodos] = useState<number[]>([]); | |
| const [loadingTodoIds, setLoadingTodoIds] = useState<number[]>([]); |
src/App.tsx
Outdated
| const newTodos = [...currentTodos]; | ||
| const index = newTodos.findIndex(todo => todo.id === updatedTodo.id); | ||
|
|
||
| newTodos.splice(index, 1, updatedTodo); | ||
|
|
||
| return newTodos; |
There was a problem hiding this comment.
Use map instead
currentTodos.map(todo => todo.id === updatedTodo.id ? updatedTodo : todo)
src/App.tsx
Outdated
| ) | ||
| .catch(error => { | ||
| setErrorMessage(ErrorMessage.UnableToDelete); | ||
| throw error; |
There was a problem hiding this comment.
You don't need to throw an error if you don't handle it on the next step. Fix in all places
src/components/ErrorNotification.tsx
Outdated
| }, 3000); | ||
|
|
||
| return () => clearTimeout(timer); | ||
| }, [error]); |
There was a problem hiding this comment.
React Hook useEffect has a missing dependency: 'setErrorMessage'. Either include it or remove the dependency array
src/components/TodoHeader.tsx
Outdated
|
|
||
| return onAddTodo(title.trim()) | ||
| .then(() => setTitle('')) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
You should handle errors. If you have done it previously you don't need a catch at all
|
|
||
| type Props = { | ||
| error: ErrorMessage; | ||
| isToogleAll: boolean | null; |
There was a problem hiding this comment.
In what case should it be null? it's a bit confusing
There was a problem hiding this comment.
in this case boolean defines 'active' button or not, but null defines is the button in the input at all or not
|
|
||
| return ( | ||
| <header className="todoapp__header"> | ||
| {isToogleAll !== null && ( |
There was a problem hiding this comment.
| {isToogleAll !== null && ( | |
| {isToogleAll && ( |
| <div | ||
| data-cy="TodoLoader" | ||
| className={cn('modal overlay', { | ||
| 'is-active': isLoading || todo.id === 0, |
There was a problem hiding this comment.
| 'is-active': isLoading || todo.id === 0, | |
| 'is-active': isLoading, |
| export const USER_ID = 2135; | ||
|
|
||
| export const getTodos = () => { | ||
| return client.get<Todo[]>(`/todos?userId=${USER_ID}`); |
There was a problem hiding this comment.
let's move /todos into const and reuse
| <a | ||
| key={status} | ||
| data-cy={`FilterLink${status}`} | ||
| href={status === 'All' ? '#/' : `#/${status.toLowerCase()}`} |
There was a problem hiding this comment.
| href={status === 'All' ? '#/' : `#/${status.toLowerCase()}`} | |
| href={status === Enum ? '#/' : `#/${status.toLowerCase()}`} |
let's use enum
DEMO