-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add task solution #1982
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
base: master
Are you sure you want to change the base?
add task solution #1982
Conversation
mateacademy-ai-mentor
left a 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.
Review Summary
Great work on structuring the application and preparing the data! Your component architecture is clean and logical.
However, I'm requesting a few changes to address some critical issues:
- Missing
keyprop: Insrc/components/CommentList/CommentList.jsx, the list of<CommentInfo>components is missing the requiredkeyprop. This violates a core React rule and a checklist item [CHECKLIST ITEM #1]. Please add a unique key, such askey={comment.id}. - Potential Crash: The
UserInfocomponent will crash if itsuserprop isnull. You need to add a check to handle this case, asgetUserByIdcan returnnull. - Incorrect Property: In
src/components/PostInfo/PostInfo.jsx, you are usingpost.name. Please check the data source, as posts usually have atitleproperty.
Once these are fixed, your solution will be ready for approval. Keep up the excellent effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/App.jsx
Outdated
| return ( | ||
| commentsFromServer.filter(comment => comment.postId === postId) || null |
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.
Array.prototype.filter() always returns an array (which is a truthy value), even if it's empty. This means the || null part of the expression will never be executed. You can simplify this function by directly returning the result of the .filter() call.
| export const CommentList = ({ comments }) => ( | ||
| <div className="CommentList"> | ||
| {comments.map(comment => ( | ||
| <CommentInfo comment={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.
This component is missing a key prop. React needs a unique key for each element in a list to efficiently update the UI. This violates checklist item #1: 'ADD a key to the root tag inside an iteration (the first tag after the .map call)'. You should add a unique key here, for example, key={comment.id}.
src/components/PostInfo/PostInfo.jsx
Outdated
| export const PostInfo = ({ post }) => ( | ||
| <div className="PostInfo"> | ||
| <div className="PostInfo__header"> | ||
| <h3 className="PostInfo__title">{post.name}</h3> |
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.
Typically, a post object from an API has a title property for its heading, not a name. While the user and comment objects in this project use a name property, using post.name here is unconventional and may not render the intended title. Please double-check the posts.json file to confirm the correct property name for the post's title.
src/components/UserInfo/UserInfo.jsx
Outdated
| export const UserInfo = () => <>Put the user here</>; | ||
| import './UserInfo.scss'; | ||
|
|
||
| export const UserInfo = ({ user }) => ( |
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 component will crash if the user prop is null because it tries to access user.email and user.name. The getUserById function can return null. You should add a check to ensure user exists before trying to access its properties. For example, you could return null or a placeholder element if the user is not available.
mateacademy-ai-mentor
left a 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.
Review Summary
Excellent progress on this PR! You've fixed almost all the issues from the previous review.
However, changes are needed because the critical issue in src/components/UserInfo/UserInfo.jsx still remains. The component does not handle cases where the user prop might be null or undefined. This will cause the application to crash if a user for a given post is not found.
Please add a check to ensure the user object exists before you try to render its properties. A simple conditional check at the beginning of the component will solve the problem:
if (!user) {
return null;
}Once this is fixed, your solution will be ready for approval. You're very close!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/components/UserInfo/UserInfo.jsx
Outdated
| export const UserInfo = ({ user }) => ( | ||
| <a className="UserInfo" href={`mailto:${user.email}`}> | ||
| {user.name} | ||
| </a> | ||
| ); |
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 component still does not handle the case where the user prop might be null or undefined, which was a key point in the last review. If getUserById fails to find a user, it returns undefined, and attempting to access user.email or user.name will crash the application.
Please add a check to ensure user exists before trying to render its properties. A simple conditional render would solve this, for example: if (!user) { return null; }.
mateacademy-ai-mentor
left a 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.
Review Summary
Excellent work! I am approving your solution. You've done a fantastic job addressing the critical issue from the previous review by adding the necessary check in the UserInfo component. The entire application is now robust, well-structured, and meets all the requirements perfectly. The componentization, data handling, and modular styling are all implemented cleanly and correctly. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
https://ValentynaITCh.github.io/react_static-list-of-posts-js/