Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core features! The form validation and state management are well-handled. However, several checklist items were violated, so I am requesting a few changes to align the code with the project's architectural guidelines. The main issues are:
- Data Preparation: The logic for finding a user for a given todo is currently in
TodoList.tsx. This data preparation should be moved to theAppcomponent to keep child components simpler. [CHECKLIST ITEM #5] - Code Duplication: The
TodoInfocomponent duplicates the display logic found inUserInfo. Please refactorTodoInfoto reuse theUserInfocomponent. Also,UserInfoshould import the sharedUsertype instead of defining its own. [CHECKLIST ITEM #2] - Variable Naming: Throughout the code, single-letter variables like
e,t, anduare used. Please replace them with more descriptive names likeevent,todo, anduserto improve readability. [CHECKLIST ITEM #1]
Once you've addressed these points, your solution will be excellent. 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
src/App.tsx
Outdated
| return value.replace(/[^^\p{Script=Latin}\p{Script=Cyrillic}\d ]+/gu, ''); | ||
| }; | ||
|
|
||
| const onTitleChange = (e: React.ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' The parameter e should have a more descriptive name, such as event. This advice also applies to the event handlers on lines 34 and 41.
src/App.tsx
Outdated
| return; | ||
| } | ||
|
|
||
| const maxId = todos.length ? Math.max(...todos.map(t => t.id)) : 0; |
There was a problem hiding this comment.
This violates checklist item #1: 'Don't use one-letter variable naming.' The parameter t in the map callback should be more descriptive, like todo. This also applies to the parameters in the callbacks on line 62 and line 110.
| } | ||
|
|
||
| export const TodoInfo = ({ todo, user }: Props) => { | ||
| const actualUser = user ?? todo.user; |
There was a problem hiding this comment.
This logic is redundant. The user prop passed from TodoList is already resolved (it's either todo.user or the result of users.find()). You can simplify this by assigning the user prop directly to actualUser.
| <a className="UserInfo" href={`mailto:${actualUser.email}`}> | ||
| {actualUser.name} | ||
| </a> |
There was a problem hiding this comment.
This implementation duplicates the functionality of the UserInfo component. This violates checklist item #2: "Don't repeat yourself". You should import and use the UserInfo component here to display the user's information.
src/components/TodoList/TodoList.tsx
Outdated
| return ( | ||
| <section className="TodoList"> | ||
| {todos.map(todo => { | ||
| const user = todo.user ?? users.find(u => u.id === todo.userId); |
There was a problem hiding this comment.
This line violates two checklist items:
-
Checklist item Solution 1-6 #5: 'Prepare data in one place (App component) and pass it to child components'. The logic to find a user for a given todo constitutes data preparation and should be done in the
Appcomponent, not here.TodoListshould be a simpler component that only focuses on rendering the data it receives. -
Checklist item Solution #1: 'Don't use one-letter variable naming'. The variable
uinusers.find(u => ...)is not descriptive. A better name would beuser.
| interface UserInfoProps { | ||
| user: { | ||
| name: string; | ||
| email: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
A User type is already defined in src/types/index.ts. To avoid duplication and ensure consistency, you should import and reuse that type here instead of defining a new shape for the user prop. This aligns with the 'Don't Repeat Yourself' principle mentioned in checklist item #2.
2 participants
No description provided.