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 #1541

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

solution #1541

wants to merge 8 commits into from

Conversation

save-v
Copy link

@save-v save-v commented Dec 19, 2024

Copy link

@Welbrn Welbrn left a 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."

@save-v save-v requested a review from Welbrn December 19, 2024 23:04
Copy link

@BudnikOleksii BudnikOleksii left a 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[]>([]);

Choose a reason for hiding this comment

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

Suggested change
const [statusChangeId, setStatusChangeId] = useState<number[]>([]);
const [loadingTodoIds, setLoadingTodoIds] = useState<number[]>([]);

One state for todo ids is enough

src/App.tsx Outdated
Comment on lines 74 to 80
let boolFilter = null;

if (filter === Filter.Active) {
boolFilter = false;
} else if (filter === Filter.Completed) {
boolFilter = true;
}

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
Comment on lines 93 to 97
let changed = prev.map(todo => (todo.id === id ? updatedTodo : todo));

if (todosState === setFilteredTodos) {
changed = filterTodosByStatus(changed, filter);
}

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 };

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)

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;

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">

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">

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">

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

Choose a reason for hiding this comment

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

// Add more methods here

@save-v save-v requested a review from BudnikOleksii December 23, 2024 19:08
Copy link

@StasSokolov1 StasSokolov1 left a 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!

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.

4 participants