-
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
solution #2888
base: master
Are you sure you want to change the base?
solution #2888
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.
Good work 👍
Left comments to improve your solution
src/App.tsx
Outdated
setTitleError(false); | ||
setUserError(false); |
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.
You don't need reset errors at first
src/App.tsx
Outdated
setUserError(true); | ||
} | ||
|
||
if (title.trim() && userId !== 0) { |
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.
You do title.trim()
and userId !== 0
comparasion twice, create variables
src/App.tsx
Outdated
|
||
// import usersFromServer from './api/users'; | ||
// import todosFromServer from './api/todos'; | ||
const findUserId = (userId: number) => |
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.
const findUserId = (userId: number) => | |
const findUserById = (userId: number) => |
As I see you are looking for user in this function not for id
src/App.tsx
Outdated
userId, | ||
}; | ||
|
||
setTodos([...todos, newTodo]); |
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.
When you set state depending on the previous one use functional setState
src/App.tsx
Outdated
setTitle(''); | ||
setUserId(0); |
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.
Create a function for resetting
src/App.tsx
Outdated
const [titleError, setTitleError] = useState<boolean>(false); | ||
const [userError, setUserError] = useState<boolean>(false); |
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.
Boolean variables should start from is
or has
src/App.tsx
Outdated
onChange={(e: ChangeEvent<HTMLInputElement>) => { | ||
setTitle(e.target.value); | ||
setTitleError(false); | ||
}} |
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.
Create functions instead of passing anonim function to onChange
events
return ( | ||
<article | ||
data-id={todo.id} | ||
className={`TodoInfo ${todo.completed ? 'TodoInfo--completed' : ''}`} |
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.
To set classes conditionally use classnames or clsx library
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.
Looks much more better now, I'm approving it🔥
DEMO LINK