develop#2039
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the overall structure and component architecture! The data preparation is handled correctly. However, I'm requesting changes because of a critical bug that can crash the application. In src/components/PostInfo/PostInfo.tsx, you access post.comments.length without checking if post.comments exists. Since it's an optional property, this will throw an error if it's undefined. Please add a safeguard, such as checking post.comments before accessing its properties.
Also, please adjust how you combine static text with components to follow our guidelines [CHECKLIST ITEM #5]. Instead of {'Posted by '}{...}, a more readable approach is to include the text directly within the JSX tag: <p>Posted by {...}</p>. Once these two points are addressed, your work will be ready to go!
✨ 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
| <p> | ||
| {' Posted by '} | ||
|
|
||
| {post.user ? <UserInfo user={post.user} /> : null} | ||
| </p> |
There was a problem hiding this comment.
This violates checklist item #5: 'Use string interpolation inside tag content'. While you can't put a component inside a template string, this approach of adding static text is a bit clunky. A more standard and readable way in JSX is to place the static text directly within the tag, like so:
<p>
Posted by {post.user ? <UserInfo user={post.user} /> : null}
</p>|
|
||
| <hr /> | ||
|
|
||
| {post.comments.length === 0 ? ( |
There was a problem hiding this comment.
The Post type declares comments as an optional property (comments?: Comment[]), meaning it could be undefined. This code accesses post.comments.length without checking if post.comments exists first, which would cause a runtime error. To make the component more robust, you should check for its existence, for example: !post.comments || post.comments.length === 0.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this PR! You've fixed the critical bugs from the previous review and the overall structure is solid. We're very close to the finish line, but a couple of adjustments are needed to fully align with the project's specific coding standards.
Here's what needs to be fixed:
-
In
src/components/PostList/PostList.tsxandsrc/components/CommentList/CommentList.tsx, thekeyprop is passed directly to a custom component. Per our guidelines, please wrap thePostInfoandCommentInfocomponents in a<div>or<React.Fragment>and apply thekeyto that wrapper element instead. [CHECKLIST ITEM #2] -
In
src/components/CommentInfo/CommentInfo.tsx, the static text' by 'is isolated in its own curly braces. For better readability, please integrate this text directly within the parentdivalong with the other elements. [CHECKLIST ITEM #5]
Once these changes are made, the solution will be ready for approval. Keep up the great 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
| return ( | ||
| <div className="CommentList"> | ||
| {comments.map(comment => ( | ||
| <CommentInfo key={comment.id} comment={comment} /> |
There was a problem hiding this comment.
The task requirements state: 'Do not pass the key prop down to the child component.' While passing a key to a custom component is valid in React, for this project, we want to adhere strictly to this rule to maintain consistency. Please wrap CommentInfo in an element (like React.Fragment) and move the key prop to that wrapping element.
| <div className="CommentInfo__title"> | ||
| <strong className="CommentInfo__name">{comment.name}</strong> | ||
|
|
||
| {' by '} |
There was a problem hiding this comment.
This violates checklist item #5. For improved readability, the static text ' by ' should be integrated directly within the parent div along with the other elements, rather than being isolated in its own curly braces on a separate line.
| return ( | ||
| <div className="PostList"> | ||
| {posts.map(post => ( | ||
| <PostInfo key={post.id} post={post} /> |
There was a problem hiding this comment.
This violates checklist item #2: 'Do not pass the key prop down to the child component.' While this is standard React practice, for this project, we want to be strict with this rule. Please wrap the PostInfo component in a container element (like a div or React.Fragment) and apply the key prop to that wrapper instead.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! You've done a great job addressing the previous feedback, and the component structure is very well-organized. To help you keep moving forward, I am approving your solution, but I have a couple of minor style points for you to consider for the future.
In PostList.tsx and CommentList.tsx, the React.Fragment wrapper isn't needed when you only have a single child component. You can simplify the code by moving the key prop directly to the PostInfo and CommentInfo components, like <PostInfo key={post.id} post={post} /> [CHECKLIST ITEM #4]. Additionally, there's a small typo in the filename src/types/imdex.ts; it should be index.ts to follow convention.
Overall, this is a fantastic effort. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <Fragment key={comment.id}> | ||
| <CommentInfo comment={comment} /> | ||
| </Fragment> |
There was a problem hiding this comment.
This Fragment is unnecessary as it only wraps a single child component. This violates checklist item #4 which advises against using a React.Fragment for a single element. You can move the key prop directly to the CommentInfo component like this:
<CommentInfo key={comment.id} comment={comment} />| <Fragment key={post.id}> | ||
| <PostInfo post={post} /> | ||
| </Fragment> |
There was a problem hiding this comment.
This Fragment is unnecessary as it only wraps a single child component. This violates checklist item #4 which advises against using a React.Fragment for a single element. You can move the key prop directly to the PostInfo component like this:
<PostInfo key={post.id} post={post} />
DEMO LINK