Solution#2040
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on structuring the application and splitting the components correctly! The data preparation logic is also well-implemented.
I'm requesting a few changes before I can approve this. There are a couple of functional bugs and style violations that need to be addressed:
- Broken Email Links: In
UserInfo.tsxandCommentInfo.tsx, thehrefattribute for email links is hardcoded. Please make it dynamic by using theemailfrom the component's props, likehref={mailto:${user.email}}. - Function Naming: The functions
getUserandgetCommentsinApp.tsxshould be renamed to be more descriptive, such asgetUserByIdandgetCommentsByPostId, as required by checklist item #6. - String Usage in JSX: In several components, you're wrapping static text in
{'...'}. This should be avoided per checklist item #5. You can write the text directly, like<strong> by </strong>.
You're very close to a perfect solution! Please address these points and resubmit. Keep up the great work!
✨ 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
|
|
||
| <b data-cy="NoCommentsMessage">No comments yet</b> | ||
| </div> | ||
| function getUser(userId: number): UsersType | null { |
There was a problem hiding this comment.
This violates checklist item #6: 'If you are searching some entity by some filter, specify it in function/method name'. Since this function finds a user by their ID, a more descriptive name would be getUserById.
|
|
||
| <p> | ||
| {' Posted by '} | ||
| function getComments(postId: number): CommentsType[] | null { |
There was a problem hiding this comment.
This violates checklist item #6: 'If you are searching some entity by some filter, specify it in function/method name'. A more descriptive name like getCommentsByPostId would better reflect that this function filters comments based on the postId.
| <div className="CommentInfo__title"> | ||
| <strong className="CommentInfo__name">{comment.name}</strong> | ||
|
|
||
| {' by '} |
There was a problem hiding this comment.
This violates checklist item #5: 'Use string interpolation inside tag content'. Using {'...'} for static text is discouraged. You can write the text directly between the JSX elements, and React will render it. For example: </strong> by <a>.
|
|
||
| {' by '} | ||
|
|
||
| <a className="CommentInfo__email" href="mailto:Nikita@garfield.biz"> |
There was a problem hiding this comment.
The href attribute for the email link is hardcoded. It should be dynamic and use the email property from the comment prop, like href={mailto:${comment.email}}.
| export const UserInfo: React.FC<Props> = ({ user }) => { | ||
| return ( | ||
| <p> | ||
| {' Posted by '} |
There was a problem hiding this comment.
This violates checklist item #5: 'Use string interpolation inside tag content'. Instead of using {'...'} for a simple string, you can write the text directly within the <p> tag. For example: <p>Posted by {user && ...}</p>.
| {' Posted by '} | ||
|
|
||
| {user !== null && ( | ||
| <a className="UserInfo" href="mailto:Julianne.OConner@kory.org"> |
There was a problem hiding this comment.
The href attribute is hardcoded. It should dynamically use the email property from the user prop to create the correct mailto link. For example: href={mailto:${user.email}}.
| import { UsersType } from './../types/UsersType'; | ||
| import { CommentsType } from './../types/CommentsType'; |
There was a problem hiding this comment.
The relative paths for these imports are a bit complex. Since UsersType.ts and CommentsType.ts are in the same directory, you can simplify the paths to ./UsersType and ./CommentsType respectively. This makes the code cleaner and easier to read.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully implemented all the core requirements, and the component structure is very well done. I am happy to approve your solution. I have a couple of small suggestions to make your code even more consistent and readable: - In src/App.tsx, consider renaming the function to getCommentsByPostId to be more specific about its purpose, as it filters by postId. [CHECKLIST ITEM #6] - In UserInfo.tsx and CommentInfo.tsx, it's a good practice to use template literals for constructing strings, like `mailto:${user.email}`. This improves consistency. [CHECKLIST ITEM #5] These are minor points for future reference. Great job overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| <p> | ||
| {' Posted by '} | ||
| function getCommentsById(postId: number): CommentsType[] | null { |
There was a problem hiding this comment.
To be more specific and fully align with the previous suggestion, please rename this function to getCommentsByPostId. This makes it clearer that the function filters comments based on the postId.
| <div className="CommentInfo__title"> | ||
| <strong className="CommentInfo__name">{comment.name}</strong> | ||
| <span> by </span> | ||
| <a className="CommentInfo__email" href={`mailto:` + comment.email}> |
There was a problem hiding this comment.
This works, but a more idiomatic way to use template literals is to embed the variable directly, like `mailto:${comment.email}`. This avoids mixing template literals with string concatenation.
| return ( | ||
| <p> | ||
| Posted by {user !== null && ( | ||
| <a className="UserInfo" href={`mailto:` + user.email}> |
There was a problem hiding this comment.
This works correctly, but for consistency with modern JavaScript practices and the feedback on other components, it's better to use a template literal here: `mailto:${user.email}`. This avoids mixing concatenation with template literals.
3 participants
No description provided.