solution #2923
solution #2923
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 🚀 While your code is mostly well-implemented, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Data Preparation and Import Usage: The
TodoInfocomponent directly importsusersFromServer, which violates the requirement to centralize data preparation in theAppcomponent. Please refactor this to pass the necessary data as props from theAppcomponent. -
New Todo ID Calculation: The current method of calculating the new todo ID using
Math.max(...ids) + 1can fail when thetodosarray is empty, resulting in an incorrect ID. Please handle the case wheretodosis empty to ensure correct ID assignment. -
userIdState Initialization: InitializinguserIdto0can lead to form submission with an invalid user ID if the selection isn't changed. Consider initializinguserIdto a non-zero value or adding explicit handling for this case.
Addressing these issues will ensure the application functions correctly and meets the core requirements. Keep up the great work, and I look forward to seeing your improvements! 😊
✨ 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.
|
|
||
| // import usersFromServer from './api/users'; | ||
| // import todosFromServer from './api/todos'; | ||
|
|
||
| export const App = () => { | ||
| const [todos, setTodos] = useState(todosFromServer as Todo[]); | ||
| const [todoTitle, setTodoTitle] = useState(''); | ||
| const [userId, setUserId] = useState(0); |
There was a problem hiding this comment.
The userId state is initialized to 0, which is also the value used for the disabled option in the user select dropdown. This means that if a user doesn't change the selection, the form will submit with userId as 0, which might not be valid. Consider initializing userId to a non-zero value or handling this case explicitly.
src/App.tsx
Outdated
| const ids = todos.map(el => el.id); | ||
|
|
||
| const newTodo = { | ||
| id: Math.max(...ids) + 1, |
There was a problem hiding this comment.
The calculation of the new todo ID using Math.max(...ids) + 1 assumes that there is at least one todo in the list. If the todos array is empty, this will result in Math.max(...ids) being -Infinity, which will cause an incorrect ID. Consider handling the case where todos is empty separately.
src/components/TodoInfo/TodoInfo.tsx
Outdated
| export const TodoInfo: React.FC<Props> = ({ todo }) => { | ||
| const { id, title, completed, userId } = todo; | ||
|
|
||
| const user = usersFromServer.find(el => el.id === userId); |
There was a problem hiding this comment.
The user object is retrieved using find, which may return undefined if no user with the matching userId is found. Although this is checked in the JSX with {user && <UserInfo user={user} />}, it's good practice to handle the case where user is undefined more explicitly, possibly with a fallback message or a warning log.
There was a problem hiding this comment.
Review Summary
Great effort on the implementation! 🎉 While the Pull Request is approved to help you move forward with your learning, there are a few critical issues that need attention:
-
Data Preparation: Ensure that
usersFromServeris not directly imported and used in components likeTodoInfo. Instead, centralize data preparation in theAppcomponent and pass the necessary data as props to child components. -
New Todo ID Calculation: The current method of calculating a new todo ID using
Math.max(...ids) + 1may fail when thetodosarray is empty. Make sure to handle this scenario to ensure correct ID assignment, starting from 1 if necessary. -
User ID Initialization: Initializing
userIdto0can lead to form submission with an invalid user ID. Consider initializing it to a non-zero value or explicitly handling this case to prevent potential issues.
These improvements are crucial for the functionality and adherence to task requirements. Keep up the good work, and make sure to address these points in future iterations to enhance your code quality and learning experience. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| import './App.scss'; | ||
| import todosFromServer from './api/todos'; | ||
| import usersFromServer from './api/users'; |
There was a problem hiding this comment.
The usersFromServer should not be directly imported and used here. According to the task requirements, data preparation should be centralized in the App component, and then passed as props to child components.
|
|
||
| // import usersFromServer from './api/users'; | ||
| // import todosFromServer from './api/todos'; | ||
|
|
||
| export const App = () => { | ||
| const [todos, setTodos] = useState(todosFromServer as Todo[]); | ||
| const [todoTitle, setTodoTitle] = useState(''); | ||
| const [userId, setUserId] = useState(0); |
There was a problem hiding this comment.
Initializing userId to 0 can lead to form submission with an invalid user ID. Consider initializing userId to a non-zero value or adding explicit handling for this case.
src/components/TodoInfo/TodoInfo.tsx
Outdated
| @@ -1 +1,25 @@ | |||
| export const TodoInfo = () => {}; | |||
| import { Todo } from '../../type/todo'; | |||
| import usersFromServer from '../../api/users'; | |||
There was a problem hiding this comment.
The usersFromServer should not be directly imported and used here. According to the task requirements, data preparation should be centralized in the App component, and then passed as props to child components like TodoInfo.
DEMO LINK