-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add task solution #1524
base: master
Are you sure you want to change the base?
add task solution #1524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GJ 👍
Added a few minor comments
src/api/todos.ts
Outdated
return client.post<Todo>('/todos', { userId, title, completed }); | ||
}; | ||
|
||
export const deleteTodos = (postId: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const deleteTodos = (postId: number) => { | |
export const deleteTodo = (todoId: number) => { |
You delete (and add, update) only one todo
Also, it's not a post - it's todo)
src/api/todos.ts
Outdated
return client.get<Todo[]>(`/todos?userId=${USER_ID}`); | ||
}; | ||
|
||
export const postTodos = ({ userId, title, completed }: Omit<Todo, 'id'>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const postTodos = ({ userId, title, completed }: Omit<Todo, 'id'>) => { | |
export const addTodo = ({ userId, title, completed }: Omit<Todo, 'id'>) => { |
you can name function depending on what does it do. It shouldn't reflect a request method in the name
src/App.tsx
Outdated
}) | ||
.catch(() => { | ||
setErrorMessage('Unable to delete a todo'); | ||
setTimeout(() => setErrorMessage(''), 3000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a separate useEffect for it - check if error exist - if yes setTimeout to clear it
src/components/Header.tsx
Outdated
setTempTodo, | ||
todosInProcess, | ||
updateTodo, | ||
newTitleTodo: newTodoTitle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can name props like it instead of renaming
src/App.tsx
Outdated
@@ -1,26 +1,158 @@ | |||
/* eslint-disable max-len */ | |||
/* eslint-disable react-hooks/rules-of-hooks */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to disable this rule? Better not do it and figure the problem out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🔥
DEMO LINK