Solution #2897
Solution #2897
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request:
-
Data Preparation: The
TodoInfocomponent importsusersFromServerdirectly, which violates the guideline of preparing data in theAppcomponent and passing it down to child components. Please modify the component to receive user data as a prop. -
Key Prop in Lists: Ensure that each child in a list has a unique
keyprop. This applies to both theTodoInfocomponents and theoptionelements within theselecttag. -
Handling Null Values: Components using the
getUserfunction should handle cases where it returnsnullto prevent potential runtime errors. -
Class Name Check: Verify that the
TodoInfocomponent correctly adds theTodoInfo--completedclass for completed todos. -
Max ID Calculation: Update the calculation of
maxIdafter adding a new task to ensure it reflects the latest state.
Please address these issues to align with the task requirements and ensure the functionality is robust. Keep up the great work, and feel free to reach out if you have any questions or need further assistance! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/App.tsx
Outdated
| setTodos(tasks); | ||
| }, []); | ||
|
|
||
| let maxId = Math.max(...todos.map((o) => o.id)) + 1; |
There was a problem hiding this comment.
The calculation of maxId should be updated after adding a new task. Consider moving this logic inside the handleSubmit function after updating the todos state.
src/App.tsx
Outdated
| Choose a user | ||
| </option> | ||
| {users.map((user) => { | ||
| return <option value={user.id}>{user.name}</option>; |
There was a problem hiding this comment.
Each child in a list should have a unique key prop. Add a key prop to the option element, using a unique value like user.id.
| completed: true, | ||
| user: user1, | ||
| }; | ||
|
|
||
| mount(<TodoInfo todo={completedTodo} />); | ||
|
|
||
| cy.get('.TodoInfo').should('have.class', 'TodoInfo--completed'); | ||
| cy.get(".TodoInfo").should("have.class", "TodoInfo--completed"); |
There was a problem hiding this comment.
Ensure that the TodoInfo component adds the TodoInfo--completed class correctly when a todo is completed. Verify that the component implementation aligns with this test expectation.
src/components/TodoInfo/TodoInfo.tsx
Outdated
| import { UserInfo } from "../UserInfo"; | ||
| import { Todo } from "../../Types/Todo"; | ||
| import "./TodoInfo.scss"; | ||
| import usersFromServer from "../../api/users"; |
There was a problem hiding this comment.
Avoid using the import keyword in non-root components for data preparation. Instead, prepare the data in the App component and pass it down as props. This aligns with the guideline: 'Prepare data in one place (App component) and pass it to child components.'
src/components/TodoList/TodoList.tsx
Outdated
| return ( | ||
| <section className="TodoList"> | ||
| {todos.map((todo) => ( | ||
| <TodoInfo todo={todo} /> |
There was a problem hiding this comment.
Each child in a list should have a unique key prop to help React identify which items have changed. Add a key prop to the TodoInfo component, using a unique value like todo.id.
| }; | ||
|
|
||
| mount(<UserInfo user={user1} />); | ||
|
|
||
| cy.get('.UserInfo').should('have.attr', 'href', 'mailto:Sincere@april.biz'); | ||
| cy.get(".UserInfo").should("have.attr", "href", "mailto:Sincere@april.biz"); |
There was a problem hiding this comment.
Ensure that the UserInfo component renders an element with the href attribute for the mailto link. If the .UserInfo class is applied to a container element, the test should target the specific element with the href attribute.
| const foundUser = usersFromServer.find((user) => user.id === userId); | ||
|
|
||
| // if there is no user with a given userId | ||
| return foundUser || null; |
There was a problem hiding this comment.
Ensure that components using the getUser function handle the case where it returns null. This is important to avoid potential runtime errors when accessing properties of a null user.
https://krzychu92.github.io/react_add-todo-form