Skip to content

sol2#3559

Open
7uckingbad wants to merge 1 commit intomate-academy:masterfrom
7uckingbad:develop
Open

sol2#3559
7uckingbad wants to merge 1 commit intomate-academy:masterfrom
7uckingbad:develop

Conversation

@7uckingbad
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.

Review Summary

Great work on this task! I am approving your solution as it successfully meets all the core functional requirements. The component structure is well-organized, and the form handling, including validation and state management, is implemented correctly.

As a point for future improvement, please pay attention to the code style guidelines. In src/App.tsx, the event handlers use a one-letter variable e. It's better practice to use more descriptive names like event to improve code readability, as noted in the checklist [CHECKLIST ITEM #1]. This is a minor point and doesn't block the approval. Keep up the great effort!


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

Footnotes

  1. Rate AI review example

})),
);

const handleSubmit = (e: FormEvent<HTMLFormElement>) => {

Choose a reason for hiding this comment

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

This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name for the event object, for example, event.

placeholder="Enter a title"
data-cy="titleInput"
value={title}
onChange={e => {

Choose a reason for hiding this comment

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

This violates checklist item #1: 'Don't use one-letter variable naming'. A more descriptive name like event would improve code readability.

id="userId"
data-cy="userSelect"
value={selectedUserId}
onChange={e => {

Choose a reason for hiding this comment

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

This violates checklist item #1: 'Don't use one-letter variable naming'. Please consider renaming e to something more descriptive like event.

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.

3 participants