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

Develop #1520

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

Develop #1520

wants to merge 5 commits into from

Conversation

DariaNastas
Copy link

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 make your code better

return;
}

// Якщо заголовок порожній, спробувати видалити туду

Choose a reason for hiding this comment

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

Remove all comments

Suggested change
// Якщо заголовок порожній, спробувати видалити туду

Comment on lines 105 to 108
onDoubleClick={() => {
setIsEditingMode(true);
setEditedTitle(todo.title);
}}

Choose a reason for hiding this comment

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

Move this logic to the helper function and use it here

return (
<footer className="todoapp__footer" data-cy="Footer">
<span className="todo-count" data-cy="TodosCounter">
{`${activeCount} item${activeCount === 1 ? '' : 's'} left`}

Choose a reason for hiding this comment

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

Move this logic to the helper variable and use it here

src/App.tsx Outdated
Comment on lines 212 to 213
onRemoveTodo={() => {}}
onToggleTodoCompletion={() => {}}

Choose a reason for hiding this comment

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

Do you need these empty props?

src/App.tsx Outdated
todo={tempTodo}
onRemoveTodo={() => {}}
onToggleTodoCompletion={() => {}}
onUpdateTodoTitle={async (todo: Todo) => todo.title}

Choose a reason for hiding this comment

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

Suggested change
onUpdateTodoTitle={async (todo: Todo) => todo.title}
onUpdateTodoTitle={(todo: Todo) => todo.title}

Comment on lines 27 to 56
<a
href="#/"
className={cn('filter__link', {
selected: filter === Filter.all,
})}
data-cy="FilterLinkAll"
onClick={() => setFilter(Filter.all)}
>
All
</a>

<a
href="#/active"
className={cn('filter__link', {
selected: filter === Filter.active,
})}
data-cy="FilterLinkActive"
onClick={() => setFilter(Filter.active)}
>
Active
</a>

<a
href="#/completed"
className={cn('filter__link', {
selected: filter === Filter.completed,
})}
data-cy="FilterLinkCompleted"
onClick={() => setFilter(Filter.completed)}
>

Choose a reason for hiding this comment

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

Use Object.values(your created enum) and render these options with map() method

src/App.tsx Outdated
Comment on lines 198 to 208
<section className="todoapp__main" data-cy="TodoList">
{filteredTodos.map(todo => (
<TodoItem
todo={todo}
key={todo.id}
onRemoveTodo={handleDeleteTodo}
onToggleTodoCompletion={handleTodoStatusChange}
onUpdateTodoTitle={handleRenameTodo}
isLoading={isLoading.includes(todo.id)}
/>
))}

Choose a reason for hiding this comment

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

Move this logic to the separate TodoList component

@DariaNastas
Copy link
Author

@volodymyr-soltys97, Thank you very much for all the recommendations!

The code has been rewritten.

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.

Great work! 🔥

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.

2 participants