-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
develop #1496
base: master
Are you sure you want to change the base?
develop #1496
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.
LGTM!
Have just a few suggestions below⬇️
src/components/TodoHeader.tsx
Outdated
// todos.forEach(todo => | ||
// onUpdate({ | ||
// ...todo, | ||
// completed: isAllCompleted ? false : true, | ||
// }), | ||
// ); |
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.
remove all comments
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 point!
|
||
const handleUpdate = () => { | ||
if (!newTitle) { | ||
setIsEditing(true); |
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.
maybe setIsEditing(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.
Fix it
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.
Edit Form
if title became empty
should stay open on fail:
return; | ||
} | ||
|
||
if (newTitle.trim() === todo.title) { |
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 can create a variable for newTitle.trim()
since you use it more than once
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 point!
src/components/TodoItem.tsx
Outdated
const [isEditing, setIsEditing] = useState(false); | ||
const [newTitle, setNewTitle] = useState(todo.title); | ||
const [isUpdating, setIsUpdating] = useState(false); | ||
const [isChecked, setIsChecked] = useState(todo.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.
why do you need the isChecked
state? I haven't seen that you use it
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.
Why do you need the isChecked
state? You can use todo.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.
Good Job 👍
let's improve your solution
src/components/TodoFooter.tsx
Outdated
return ( | ||
<footer className="todoapp__footer" data-cy="Footer"> | ||
<span className="todo-count" data-cy="TodosCounter"> | ||
{`${activeTodosCount} items left`} |
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.
{`${activeTodosCount} items left`} | |
{activeTodosCount} items left |
completedTodosIds: number[]; | ||
} | ||
|
||
export const TodoFooter: React.FC<Props> = ({ |
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.
export const TodoFooter: React.FC<Props> = ({ | |
export const TodoFooter: FC<Props> = ({ |
src/components/TodoHeader.tsx
Outdated
// todos.forEach(todo => | ||
// onUpdate({ | ||
// ...todo, | ||
// completed: isAllCompleted ? false : true, | ||
// }), | ||
// ); |
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 point!
src/components/TodoItem.tsx
Outdated
const [isEditing, setIsEditing] = useState(false); | ||
const [newTitle, setNewTitle] = useState(todo.title); | ||
const [isUpdating, setIsUpdating] = useState(false); | ||
const [isChecked, setIsChecked] = useState(todo.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.
Why do you need the isChecked
state? You can use todo.completed
|
||
const handleUpdate = () => { | ||
if (!newTitle) { | ||
setIsEditing(true); |
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.
Fix it
return; | ||
} | ||
|
||
if (newTitle.trim() === todo.title) { |
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 point!
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.
Well done 🔥
https://Ostkreuzzz.github.io/react_todo-app-with-api/