-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
react_dynamic-list-of-posts solution #874
base: master
Are you sure you want to change the base?
Conversation
NataliGru
commented
Aug 2, 2023
•
edited
Loading
edited
- DEMO LINK
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!
I found some issues, please check and fix them
Best regards
Add | ||
</button> | ||
</div> | ||
|
||
<div className="control"> | ||
{/* eslint-disable-next-line react/button-has-type */} | ||
<button type="reset" className="button is-link is-light"> | ||
<button |
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 are using 'eslint-disable-next-line react/button-has-type' to disable the linter warning. Instead of disabling the linter, try to solve the issue. If the linter is giving a warning, it's probably for a good reason.
}) | ||
.finally(() => { | ||
setIsLoading(false); | ||
setBody(''); |
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.
In the finally block of addComment, it only clears the body state, it should also clear name and email states for consistency.
setComments(prev => prev.filter(comm => comm.id !== commentId)); | ||
|
||
postService.deleteComment(commentId) | ||
.catch(() => { |
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.
In case of an error in deleting the comment, only 'postError' is set to true and the 'comments' state is reverted. It would be better to show a descriptive error message to the user.
const deleteComment = (commentId: number) => { | ||
const currentComments = comments; | ||
|
||
setComments(prev => prev.filter(comm => comm.id !== commentId)); |
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.
Optimistic update is performed by directly modifying the state before the result of the delete request is known. In case of an error, this could lead to data inconsistency between client and server.
}, [post]); | ||
|
||
const deleteComment = (commentId: number) => { | ||
const currentComments = comments; |
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 'currentComments' is defined but it is not updated when the comments state changes. This may lead to using stale data when an error occurs during comment deletion.
}; | ||
|
||
const onAddComment = (comment: Comment) => { | ||
setComments(prev => [...prev, comment]); |
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.
Optimistic update is performed by directly modifying the state before the result of the add comment request is known. In case of an error, this could lead to data inconsistency between client and server.
|
||
<Loader /> | ||
{selectedUser && isLoading && <Loader />} |
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 loader should also show when fetching users. Consider updating this condition.
|
||
if (selectedUser) { | ||
postService | ||
.getPosts(selectedUser?.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.
The optional chaining in 'selectedUser?.id' is not needed since you've already checked if 'selectedUser' is truthy in the if condition above.
setError(false); | ||
|
||
postService | ||
.getUsers() |
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 are not handling the loading state for fetching users. Consider updating the 'isLoading' state variable before and after the users fetch request.