-
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
done #2905
base: master
Are you sure you want to change the base?
done #2905
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 your implementation! 🎉 The code is well-structured and meets most of the task requirements. While there are a few areas for improvement, such as handling potential null
values in the getUserById
function and considering special characters in the validateTitleChange
function, these are minor issues that don't significantly impact the core functionality. Keep up the good work, and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
const initialTodos: Todos[] = todosFromServer.map(todo => ({ | ||
...todo, | ||
user: getUserById(todo.userId), |
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 getUserById
function can return null
if a user is not found. Ensure that the user
property in the initialTodos
array handles this case to avoid potential runtime errors.
title: titleInput.trim(), | ||
completed: false, | ||
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 getUserById
function can return null
, which would result in a null
user object in the new todo. Consider adding a check to handle this scenario to prevent potential errors.
}; | ||
|
||
const validateTitleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
const checkValues = event.target.value.replace(/[^a-zA-Zа-яА-Я0-9 ]/g, ''); |
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 validateTitleChange
function currently removes all non-alphanumeric characters and spaces. If special characters are allowed in the title, consider adjusting the regex to accommodate them.
> | ||
<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.
The UserInfo
component is used here with the assumption that todo.user
is always defined. If todo.user
can be null
or undefined
, consider adding a conditional check to handle these cases to prevent potential runtime errors.
No description provided.