Solution #1237
Solution #1237
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your code is generally well-structured and follows best practices, there are a few critical issues that need to be addressed before we can approve it:
-
Error Handling: It's important to implement
try...catchblocks for error handling in theuseEffecthooks and functions likeaddCommentanddeleteComment. This is crucial for ensuring that your application can gracefully handle errors and maintain functionality. -
Event Handling: The
onClickhandler for the 'Clear' button inNewCommentForm.tsxshould be a function reference rather than an immediate function call. This is necessary to ensure the button behaves as expected.
These changes are essential to meet the core requirements and ensure the robustness of your application. Once these issues are resolved, your code will be in great shape! Keep up the great work, and I look forward to seeing the improvements. 🚀
✨ 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
| useEffect(() => { | ||
| setLoading(CurLoading.Users); | ||
| getUsers() | ||
| .then(users => { | ||
| setAllUsers(users); | ||
| setErrorMessage(CurError.Empty); | ||
| }) | ||
| .catch(() => setErrorMessage(CurError.LoadUsers)) | ||
| .finally(() => setLoading(CurLoading.Empty)); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
It's recommended to use a try...catch block for error handling in the useEffect hook. This aligns with the checklist's optional recommendation to use try {..} catch for error handling.
src/App.tsx
Outdated
| className={classNames( | ||
| 'tile', | ||
| 'is-parent', | ||
| 'is-8-desktop', | ||
| 'Sidebar', | ||
| { | ||
| 'Sidebar--open': | ||
| activePost && activePost.userId === selectedUser?.id, | ||
| }, | ||
| )} |
There was a problem hiding this comment.
Ensure consistent use of the classnames library for defining classes conditionally. This is an optional recommendation from the checklist, but it can improve code readability and maintainability.
src/components/NewCommentForm.tsx
Outdated
| onClick={clearComment( | ||
| setName, | ||
| setEmail, | ||
| setText, | ||
| setErrorMessage, | ||
| setErrorName, | ||
| setErrorEmail, | ||
| setErrorText, | ||
| )} |
There was a problem hiding this comment.
The onClick handler for the 'Clear' button should be a function. Currently, clearComment is being called immediately, which is incorrect. It should be passed as a function reference: onClick={() => clearComment(...)}.
src/components/PostDetails.tsx
Outdated
| useEffect(() => { | ||
| setLoading(CurLoading.Comms); | ||
| if (activePost) { | ||
| getCommentsByPostId(activePost.id) | ||
| .then(commsFromServer => setComments(commsFromServer)) | ||
| .catch(() => setErrorMessage(CurError.LoadComs)) | ||
| .finally(() => setLoading(CurLoading.Empty)); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [activePost]); |
There was a problem hiding this comment.
It's recommended to use a try...catch block for error handling in the useEffect hook. This aligns with the checklist's optional recommendation to use try {..} catch for error handling.
src/utils/servises.ts
Outdated
| export const deleteComment = async ( | ||
| comId: number, | ||
| setComments: React.Dispatch<React.SetStateAction<Comment[]>>, | ||
| setLoading: React.Dispatch<React.SetStateAction<CurLoading>>, | ||
| setErrorMessage: React.Dispatch<React.SetStateAction<CurError | string>>, | ||
| ) => { | ||
| setLoading(CurLoading.DelComs); | ||
|
|
||
| return client | ||
| .delete(`/comments/${comId}`) | ||
| .then(() => { | ||
| setComments(latestComms => latestComms.filter(com => com.id !== comId)); | ||
| }) | ||
| .catch(() => setErrorMessage(CurError.DelComs)) | ||
| .finally(() => setLoading(CurLoading.Empty)); | ||
| }; |
There was a problem hiding this comment.
Consider using a try...catch block for error handling in the deleteComment function. This aligns with the checklist's optional recommendation to use try {..} catch for error handling.
src/utils/servises.ts
Outdated
| export const addComment = async ( | ||
| activePost: Post | null, | ||
| name: string, | ||
| email: string, | ||
| text: string, | ||
| comments: Comment[], | ||
| setComments: React.Dispatch<React.SetStateAction<Comment[]>>, | ||
| setText: React.Dispatch<React.SetStateAction<string>>, | ||
| setLoading: React.Dispatch<React.SetStateAction<CurLoading>>, | ||
| setNewComment: React.Dispatch<React.SetStateAction<CommentData | null>>, | ||
| setErrorMessage: React.Dispatch<React.SetStateAction<CurError | string>>, | ||
| ) => { | ||
| const id = | ||
| comments.length === 0 ? 1 : Math.max(...comments.map(com => com.id)) + 1; | ||
|
|
||
| const newComment: Comment = { | ||
| id, | ||
| postId: activePost!.id, | ||
| name, | ||
| email, | ||
| body: text, | ||
| }; | ||
|
|
||
| const commentData = { | ||
| postId: newComment.postId, | ||
| name: newComment.name, | ||
| email: newComment.email, | ||
| body: newComment.body, | ||
| }; | ||
|
|
||
| setLoading(CurLoading.AddComs); | ||
|
|
||
| return client | ||
| .post(`/comments`, commentData) | ||
| .then(() => { | ||
| setComments(prev => [...prev, newComment]); | ||
| setNewComment(commentData); | ||
| setText(''); | ||
| }) | ||
| .catch(() => setErrorMessage(CurError.AddComs)) | ||
| .finally(() => setLoading(CurLoading.Empty)); | ||
| }; |
There was a problem hiding this comment.
Consider using a try...catch block for error handling in the addComment function. This aligns with the checklist's optional recommendation to use try {..} catch for error handling.
There was a problem hiding this comment.
Review Summary
Great job on addressing the previous issues related to error handling and event handling! 🎉 Your code is well-structured and aligns with React best practices, particularly in the use of useEffect, context, and state management. Here are some key points and suggestions for improvement:
-
Error Handling: You've effectively implemented
try...catchblocks in youruseEffecthooks and functions likeaddCommentanddeleteComment, ensuring graceful error handling. -
Event Handling: The
onClickhandler for the 'Clear' button inNewCommentForm.tsxshould be a function reference rather than an immediate function call. This will ensure the button behaves as expected and only executes theclearCommentfunction when clicked. -
Code Structure: Your components are well-structured and make good use of context for state management. Ensure that all imported modules and components exist and are correctly implemented in their respective files.
-
Utility Functions: The
servises.tsfile is well-organized, with appropriate use of async/await and error handling. Make sure theclientobject is correctly implemented infetchClient.
Your code meets the task requirements and constraints, and the decision to approve is made to help you move forward with your learning. Please review the comments to improve your code further. Addressing the critical issue with the onClick handler will enhance the functionality of your application. 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! ✨
| onClick={() => | ||
| clearComment( | ||
| setName, | ||
| setEmail, | ||
| setText, | ||
| setErrorMessage, | ||
| setErrorName, | ||
| setErrorEmail, | ||
| setErrorText, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The onClick handler for the 'Clear' button should be a function reference. Currently, it is an immediate function call, which can lead to unexpected behavior. Consider wrapping the clearComment call in a function to ensure it is only executed when the button is clicked.
https://sborichevskyi.github.io/react_dynamic-list-of-posts/