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

Done #1572

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

Done #1572

wants to merge 6 commits into from

Conversation

IrynaMariiko00
Copy link

@IrynaMariiko00 IrynaMariiko00 commented Dec 26, 2024

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your code
You need to fix this failed test, if you need help feel free in the fe_chat
image

README.md Outdated
@@ -47,4 +47,4 @@ Implement the ability to edit a todo title on double click:

- Implement a solution following the [React task guideline](https://github.com/mate-academy/react_task-guideline#react-tasks-guideline).
- Use the [React TypeScript cheat sheet](https://mate-academy.github.io/fe-program/js/extra/react-typescript).
- Replace `<your_account>` with your Github username in the [DEMO LINK](https://<your_account>.github.io/react_todo-app-with-api/) and add it to the PR description.
- Replace `<your_account>` with your Github username in the [DEMO LINK](https:/IrynaMariiko00.github.io/react_todo-app-with-api/) and add it to the PR description.

Choose a reason for hiding this comment

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

The demo link in the PR description is wrong now, add link from this file
image

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

don't forget to add loaders on clear completed or toggle all click

src/App.tsx Outdated
Comment on lines 119 to 123
useEffect(() => {
const allCompleted = todos.every(todo => todo.completed);

setIsToggleAllActive(allCompleted);
}, [todos]);

Choose a reason for hiding this comment

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

Suggested change
useEffect(() => {
const allCompleted = todos.every(todo => todo.completed);
setIsToggleAllActive(allCompleted);
}, [todos]);
const isToggleAllActive = todos.every(todo => todo.completed);

just can be a var

Iryna Mariiko added 2 commits December 27, 2024 12:09
Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Looks good.

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