Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the core functionality! The form submission and validation logic are working correctly. However, one change is required to align with the project's coding standards. In src/App.tsx, you've used a one-letter variable e for event handlers, which violates our checklist rule about descriptive variable naming [CHECKLIST ITEM #1]. Please rename it to something more descriptive, like event.
I also noticed a couple of minor things you could clean up:
- Remove the large block of commented-out code in
src/components/UserInfo/UserInfo.tsx. - Remove the redundant
keyprop from the<article>element insidesrc/components/TodoInfo/TodoInfo.tsx.
✨ 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
| placeholder="Enter a title" | ||
| data-cy="titleInput" | ||
| value={title} | ||
| onChange={e => { |
There was a problem hiding this comment.
This violates checklist item #1: "Don't use one-letter variable naming." Please use a more descriptive name for the event object, such as event or changeEvent.
src/App.tsx
Outdated
| id="userId" | ||
| data-cy="userSelect" | ||
| value={selectedUserId} | ||
| onChange={e => { |
There was a problem hiding this comment.
This violates checklist item #1: "Don't use one-letter variable naming." Please use a more descriptive name for the event object, like event.
src/components/UserInfo/UserInfo.tsx
Outdated
| // <div className="field"> | ||
| // <label htmlFor="user">User: </label> | ||
|
|
||
| // <select | ||
| // id="user" | ||
| // name="user" | ||
| // data-cy="userSelect" | ||
| // defaultValue={user} | ||
| // onChange={event => { | ||
| // setUser(+event.target.value); | ||
| // isError = false; | ||
| // }} | ||
| // > | ||
| // <option value={0} disabled> | ||
| // Choose a user | ||
| // </option> | ||
|
|
||
| // {usersFromServes.map(person => ( | ||
| // <option value={person.id + 1} key={person.id}> | ||
| // {person.name} | ||
| // </option> | ||
| // ))} | ||
| // </select> | ||
|
|
||
| // {isError && <span className="error">Please choose a user</span>} | ||
| // </div> |
There was a problem hiding this comment.
This large block of commented-out code should be removed. It's not used and makes the component harder to read.
| className={classNames('TodoInfo', { | ||
| 'TodoInfo--completed': todo.completed, | ||
| })} | ||
| key={todo.id} |
There was a problem hiding this comment.
The key prop is used by React to identify which items have changed, are added, or are removed in a list. It should be applied to the component when it is rendered within a map function (as is correctly done in TodoList.tsx). Placing the key prop on an element inside the component itself is redundant and has no effect, as key is a special prop that is not passed down to the component's children.
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 PR! You've successfully implemented all the core requirements and addressed the most important feedback from the last review. I am approving your solution.
I have two minor suggestions for you to consider for future improvements:
- In
src/App.tsx, there is a small typo wherepresonshould beperson. - In
src/components/TodoInfo/TodoInfo.tsx, thekeyprop on the<article>element is redundant. Since the key is already correctly applied where you map over the components inTodoList.tsx, it's not needed insideTodoInfoitself.
These are just small cleanups, and the functionality is perfect. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| import { User } from './types/User'; | ||
|
|
||
| function getUserById(userId: number): User { | ||
| const user = usersFromServer.find(preson => preson.id === userId); |
There was a problem hiding this comment.
This is a minor typo. It looks like preson should be person for better clarity.
| className={classNames('TodoInfo', { | ||
| 'TodoInfo--completed': todo.completed, | ||
| })} | ||
| key={todo.id} |
There was a problem hiding this comment.
This key prop is redundant. The key is already being correctly applied to the <TodoInfo /> component where it's mapped in the TodoList component. The key should be on the elements in the array, not on the root element inside the component itself.
2 participants
No description provided.