add solution #2913
add solution #2913
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your implementation meets many of the task requirements, there are a couple of critical issues that need to be addressed before we can approve it:
-
Data Handling in Non-root Components: The
UserInfocomponent importsusersFromServerdirectly, which violates the guideline to prepare data in the App component and pass it to child components. This is an important best practice for maintaining a clean and scalable codebase. Please refactor your code to pass the necessary data from the App component. -
Variable Naming: In your event handlers, consider using more descriptive variable names instead of one-letter names like
e. This will greatly enhance the readability and maintainability of your code.
Once these changes are made, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you have any questions. 🌟
✨ 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
|
|
||
| const [todos, setTodos] = React.useState<Todo[]>(todosFromServer); | ||
|
|
||
| const handleTitleChange = (e: React.ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
Avoid using one-letter variable names like e for event handlers. Instead, use a more descriptive name such as event to improve code readability and maintainability. This aligns with the checklist item: "Don't use one-letter variable naming."
src/App.tsx
Outdated
| setTitleError(false); | ||
| }; | ||
|
|
||
| const handleSelectChange = (e: React.ChangeEvent<HTMLSelectElement>) => { |
There was a problem hiding this comment.
Consider renaming the variable e to a more descriptive name like event in the handleSelectChange function. This will enhance code readability and is recommended by the checklist: "Don't use one-letter variable naming."
| export const TodoInfo = () => {}; | ||
| import React from 'react'; | ||
| import cn from 'classnames'; | ||
| import { UserInfo } from '../UserInfo'; |
There was a problem hiding this comment.
According to the checklist, you should not use the import keyword in non-root components. Instead, prepare data in the App component and pass it to child components. Consider passing the necessary data for UserInfo from the App component to TodoInfo and then to UserInfo to adhere to this guideline.
src/components/UserInfo/UserInfo.tsx
Outdated
| @@ -1 +1,29 @@ | |||
| export const UserInfo = () => {}; | |||
| import React from 'react'; | |||
| import usersFromServer from '../../api/users'; | |||
There was a problem hiding this comment.
Avoid importing data directly in non-root components. Instead, prepare the data in the App component and pass it down to child components. This aligns with the checklist item: "Don't use the import keyword in non-root components. Prepare data in the App component and pass it to child components."
There was a problem hiding this comment.
Great job on addressing the majority of the issues from the previous review! 🎉 Your code is well-structured and demonstrates a solid understanding of React components and TypeScript types. Here are a few points to consider for further improvement:
-
Data Handling: Ensure that all data preparation is done in the
Appcomponent and passed down to child components as props. This is crucial for maintaining a clean and manageable codebase. -
Variable Naming: Consider using more descriptive names for event handler variables in the
Formcomponent to enhance code readability. -
Error Handling: Make sure that error messages for
titleErrorandselectErrorare correctly displayed when the form is submitted with invalid inputs. -
Initialization of
select: Consider initializingselectwithnullorundefinedinstead of0to better represent an unselected state, as0might be a valid user ID. -
Return Values in
map: Returningundefinedfrom themapfunction can lead to rendering issues. Consider returningnullinstead.
These improvements will help you write cleaner and more maintainable code. Keep up the great work, and continue to learn from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const [title, setTitle] = React.useState(''); | ||
| const [titleError, setTitleError] = React.useState(false); | ||
|
|
||
| const [select, setSelect] = React.useState(0); |
There was a problem hiding this comment.
Consider initializing select with null or undefined instead of 0 to better represent an unselected state, as 0 might be a valid user ID.
| setTitleError(!title); | ||
| setSelectError(!select); |
There was a problem hiding this comment.
The error state for titleError and selectError is set based on the current values of title and select. Ensure that the error messages are correctly displayed when the form is submitted with invalid inputs.
| if (!title || !select) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The condition if (!title || !select) checks for empty title or unselected user. Ensure that select is initialized to a value that accurately represents an unselected state, as mentioned earlier.
| export const TodoList = () => {}; | ||
| import React from 'react'; | ||
| import { TodoInfo } from '../TodoInfo'; | ||
| import { Todos } from '../../types/Todos'; |
There was a problem hiding this comment.
Ensure that the Todos type correctly represents the props being passed to the TodoList component. It should include both todos and users arrays.
| ); | ||
|
|
||
| if (!user) { | ||
| return; |
There was a problem hiding this comment.
Returning undefined from the map function when no user is found can lead to issues. Consider returning null instead to avoid rendering issues.
DEMO LINK