-
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
MVP #795
base: master
Are you sure you want to change the base?
MVP #795
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.
Great job indeed.
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.
Good job, but there is quite a bit of work to be done.
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.
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.
- When I renamed my todo to empty line -> my todo was deleted, that's correct. But I got an error message on UI(you can check my comment below why that happens)
- "All" "Active" "Completed" buttons should not "jump"(check the working version on app)
- When in offline mode I try to save empty title, message should be "Unable to delete a todo"
- When I click
toggleAll
, why I see loader on every todo, if I'm updating just 1
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.
Good job 👍
Left some suggestions, take a look
setTodos(prevTodos => [...prevTodos, addedTodo]); | ||
|
||
return true; | ||
} catch { | ||
setError(ErrorsMessages.Add); | ||
|
||
return false; |
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.
setTodos(prevTodos => [...prevTodos, addedTodo]); | |
return true; | |
} catch { | |
setError(ErrorsMessages.Add); | |
return false; | |
setTodos(prevTodos => [...prevTodos, addedTodo]); | |
} catch { | |
setError(ErrorsMessages.Add); |
It's strange to return true or false in handler
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.
this is necessary in order to know whether our promise was fulfilled normally or with an error. I don't know how to do it differently, but it works)
return true; | ||
} catch { | ||
setError(ErrorsMessages.Update); | ||
|
||
return false; |
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.
The same get rid of them
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.
Well done 👍
DEMO LINK