Skip to content
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

solution #2914

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Dushkaaa-BabyBoss
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 task! 🎉 While there are a few minor issues, such as the incorrect calculation of the new id, a typo in the state variable name, and the handling of missing user data, these do not significantly impact the functionality. Keep up the good work and consider these suggestions for future improvements! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

const [userId, setUserId] = useState(0);

const [titleError, setTitleError] = useState(false);
const [useError, setUseError] = useState(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the state variable name useError. It should likely be userError to accurately reflect its purpose of handling user selection errors.

}

const newTodo: Todo = {
id: Math.max(...todos.map(todo => todo.id + 1)),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation for the new id is incorrect. The expression Math.max(...todos.map(todo => todo.id + 1)) will add 1 to each id before finding the maximum, which is not the intended behavior. Instead, find the maximum id first, then add 1 to it.

Comment on lines +36 to +37
name: user?.name || 'Empty',
email: user?.email || '',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user is not found, the default name is set to 'Empty' and the email to an empty string. While this handles the case where a user might not be found, it might be more informative to use a placeholder like 'Unknown User' or 'No Email' to make it clear that the user data is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants