-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add TODO Form #2915
base: master
Are you sure you want to change the base?
add TODO Form #2915
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the todo list application! 🎉 Your code meets the task requirements and constraints effectively. While there are a few areas for improvement, such as ensuring proper type usage for userSelected
and checking for null values, these are minor and don't impact the core functionality. Keep up the good work, and consider the feedback for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
const [todosList, setTodosList] = useState(initialTodos); | ||
|
||
const [titleInput, setTitleInput] = useState(''); | ||
const [userSelected, setUserSelected] = useState(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The userSelected
state is initialized as an empty string, but it is later used as a number in the createNewTodo
function. Consider initializing it as a number or converting it to a number when setting the state to avoid potential type issues.
userId: +userSelected, | ||
user: getUserById(+userSelected), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The userId
and user
fields in the createNewTodo
function are derived from userSelected
, which is a string. Ensure userSelected
is converted to a number before using it here to avoid type mismatches.
value={userSelected} | ||
onChange={handleSelectChange} | ||
> | ||
<option value="" disabled={userSelected !== ''}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The disabled
attribute for the option element is conditionally set based on userSelected
. This logic might be unnecessary since the default option should always be disabled to prompt user selection.
> | ||
<h2 className="TodoInfo__title">{todo.title}</h2> | ||
|
||
<UserInfo user={todo.user} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the todo.user
is not null or undefined before passing it to the UserInfo
component. If todo.user
can be null, consider adding a conditional rendering check or providing a default value to prevent runtime errors.
export const TodoList: React.FC<Props> = ({ todos }) => { | ||
return ( | ||
<section className="TodoList"> | ||
{todos.map(todo => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a check to ensure that the todos
array is not null or undefined before mapping over it. This will prevent potential runtime errors if the todos
prop is not properly initialized.
DEMO LINK