add dynamic list of post task #1206
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the order of .then and .catch in the getCommentsByPostId function needs to be corrected to ensure proper error handling. Additionally, the return statement in PostDetails.tsx should return null when currentPost is not available. Lastly, please remove the unnecessary comments prop from NewCommentForm and correct the typo in resetAllFeilds. Once these issues are resolved, your code will be in great shape! Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/App.tsx
Outdated
| .getComments(postId) | ||
| .catch(() => setIsError(true)) | ||
| .then(res => { | ||
| setComments(res); | ||
| setIsError(false); | ||
| }) |
There was a problem hiding this comment.
The order of .then and .catch in the getCommentsByPostId function is incorrect. According to the checklist, .then should come before .catch to handle the resolved promise before handling any errors. Please switch the order of these methods to ensure proper error handling.
src/components/NewCommentForm.tsx
Outdated
| function resetAllFeilds() { | ||
| setName(''); | ||
| setEmail(''); | ||
| setBody(''); | ||
| } |
There was a problem hiding this comment.
There is a typo in the function name resetAllFeilds. It should be corrected to resetAllFields to maintain consistency and clarity in the code.
src/components/PostDetails.tsx
Outdated
| if (!currentPost) { | ||
| return; |
There was a problem hiding this comment.
The return statement when currentPost is not available should return null instead of nothing. In React, components should return a valid React element or null to avoid rendering issues.
src/components/PostDetails.tsx
Outdated
| <NewCommentForm | ||
| addComment={addComment} | ||
| postId={currentPost.id} | ||
| comments={comments} |
There was a problem hiding this comment.
The comments prop is being passed to NewCommentForm, but this component does not require it. Please remove this prop to avoid unnecessary data passing.
There was a problem hiding this comment.
Great job on addressing the issues from the previous review! 🎉 Your code now meets the task requirements and constraints. Here are some key points to focus on for further improvement:
- Error Handling: Ensure the
.catchblock is placed before the.thenblock in thegetCommentsByPostIdfunction for better error handling. - Type Definitions: The
addCommentfunction should not expect aPromise<void>return type since it does not return a promise. Adjust the type definition accordingly. - Handling
currentPost: ReturnnullwhencurrentPostis not found to prevent rendering issues. - Comment ID Handling: The
idfield should not be included when adding a new comment, as it should be generated by the backend. - Reset Functionality: The
resetfunction inNewCommentFormis redundant given theresetAllFieldsfunction. Consider removingresetor usingresetAllFieldsinstead.
Your efforts to improve the code are commendable, and these adjustments will enhance the application's functionality and robustness. Keep up the great work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| return servicesComments | ||
| .getComments(postId) | ||
| .then(res => { | ||
| setComments(res); | ||
| setIsError(false); | ||
| }) | ||
| .catch(() => setIsError(true)) | ||
| .finally(() => setLoading('')); |
There was a problem hiding this comment.
The .catch block should be placed before the .then block in the getCommentsByPostId function to ensure proper error handling. This will allow the function to handle errors before attempting to process the response.
| }; | ||
|
|
||
| const currentPost = useMemo(() => { | ||
| return posts.find((post: Post) => post.id === currentPostId); |
There was a problem hiding this comment.
Consider returning null when currentPost is not found. This will prevent potential issues when currentPost is used elsewhere in the code.
| function reset() { | ||
| setBody(''); | ||
| } |
There was a problem hiding this comment.
The reset function only resets the body field, which is redundant given the resetAllFields function. Consider removing reset or using resetAllFields instead.
| import { Comment } from '../types/Comment'; | ||
|
|
||
| type Props = { | ||
| addComment: (a: Comment) => Promise<void>; |
There was a problem hiding this comment.
The addComment function should not expect a Promise<void> return type since it does not return a promise. Adjust the type definition accordingly.
| email, | ||
| body, | ||
| postId, | ||
| id: 0, |
There was a problem hiding this comment.
The id field should not be included when adding a new comment, as it should be generated by the backend. Consider removing id: 0 from the addComment call.
| type Props = { | ||
| comments: Comment[]; | ||
| currentPost: Post; | ||
| addComment: (a: Comment) => Promise<void>; |
There was a problem hiding this comment.
The addComment function should not expect a Promise<void> return type since it does not return a promise. Adjust the type definition accordingly.
| if (!currentPost) { | ||
| return null; |
There was a problem hiding this comment.
Ensure that currentPost is handled correctly when it is not available by returning null. This prevents rendering issues when currentPost is undefined.
https://moskkat40.github.io/react_dynamic-list-of-posts/