-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 #1541
base: master
Are you sure you want to change the base?
solution #1541
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 job! However, when I press the 'Toggle All' button in the header, it changes the status of the items, but does not reflect the change visually. Additionally, the update is only applied locally and not on the server."
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 👍
Let's improve and simplify your solution
src/App.tsx
Outdated
|
||
const [inputText, setInputText] = useState(''); | ||
const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); | ||
const [statusChangeId, setStatusChangeId] = useState<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 [statusChangeId, setStatusChangeId] = useState<number[]>([]); | |
const [loadingTodoIds, setLoadingTodoIds] = useState<number[]>([]); |
One state for todo ids is enough
src/App.tsx
Outdated
let boolFilter = null; | ||
|
||
if (filter === Filter.Active) { | ||
boolFilter = false; | ||
} else if (filter === Filter.Completed) { | ||
boolFilter = 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.
Use switch case instead. And you can simplify this condition
src/App.tsx
Outdated
let changed = prev.map(todo => (todo.id === id ? updatedTodo : todo)); | ||
|
||
if (todosState === setFilteredTodos) { | ||
changed = filterTodosByStatus(changed, filter); | ||
} |
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.
I have no idea what is happening here. Try to simplify
src/App.tsx
Outdated
} | ||
|
||
function handleTitleChange(newTitle: string, editingTodoId: number | null) { | ||
const updateStatus = { title: newTitle }; |
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 call it status when actually store a title?
src/App.tsx
Outdated
const updateStatus = { title: newTitle }; | ||
|
||
return client | ||
.patch<Todo>(`/todos/${editingTodoId}`, updateStatus) |
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 to do it. Your component shouldn't know anything about URL where you are sending the request
src/App.tsx
Outdated
}); | ||
|
||
function changeStatusAll() { | ||
const status = 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.
It's already boolean
src/App.tsx
Outdated
<h1 className="todoapp__title">todos</h1> | ||
|
||
<div className="todoapp__content"> | ||
<header className="todoapp__header"> |
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 header component
src/App.tsx
Outdated
/> | ||
</form> | ||
</header> | ||
<section className="todoapp__main" data-cy="TodoList"> |
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 TodoList component
src/App.tsx
Outdated
|
||
{/* Hide the footer if there are no todos +++*/} | ||
{todos.length !== 0 && ( | ||
<footer className="todoapp__footer" data-cy="Footer"> |
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 Footer component
src/api/todos.ts
Outdated
return client.get<Todo[]>(`/todos?userId=${USER_ID}`); | ||
}; | ||
|
||
// Add more methods here |
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.
// Add more methods here
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.
It looks much more better, I'm approving it!
https://save-v.github.io/react_todo-app-with-api/