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

todo app final part #781

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

Conversation

AnVBondar
Copy link

@@ -0,0 +1,92 @@
/* eslint-disable @typescript-eslint/no-unused-vars */

Choose a reason for hiding this comment

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

why do you need unused-vars ?

Comment on lines 74 to 78
const sortAllInSelect = todos.filter(({ completed }) => !completed);

preparedList = sortAllInSelect.map(item => {
return { ...item, completed: true };
});

Choose a reason for hiding this comment

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

you can combine map and filter with using method reduce

Comment on lines 53 to 57
if (updatedTitle) {
updatedTodo = { ...todo, title: updatedTitle };
} else {
updatedTodo = { ...todo, completed: !completed };
}

Choose a reason for hiding this comment

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

make sense to use ternary operator here as it will make a code shorter

Comment on lines 62 to 69
setTodos(currentTodos => {
const newTodos = [...currentTodos];
const index = newTodos.findIndex(item => item.id === updatedTodo.id);

newTodos.splice(index, 1, updatedTodo);

return newTodos;
});

Choose a reason for hiding this comment

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

you use method filter here and it will make a code shorter

setIsLoading(false);
setIsEditMode(false);
} catch {
setIsLoading(false);

Choose a reason for hiding this comment

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

  • you need to call setIsLoading only once in block finally
Screenshot 2023-09-21 at 20 25 35

)}

<div className={cn('modal overlay', {
'is-active': isLoading || selectedAreDeleting(),

Choose a reason for hiding this comment

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

please don't call a function from the template because it will be executed every time when the state change, so make sense to move it to the variable

<span className="todo__title">{tempTodo.title}</span>
<button type="button" className="todo__remove">×</button>

{/* 'is-active' class puts this modal on top of the todo */}

Choose a reason for hiding this comment

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

remove all comments

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.

3 participants