Skip to content

Implement logic and functionality in TodoApp#1259

Open
TaniaFedortsi wants to merge 2 commits intomate-academy:masterfrom
TaniaFedortsi:develop
Open

Implement logic and functionality in TodoApp#1259
TaniaFedortsi wants to merge 2 commits intomate-academy:masterfrom
TaniaFedortsi:develop

Conversation

@TaniaFedortsi
Copy link

Copy link

@Denys-Kravchuk9988 Denys-Kravchuk9988 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!

A few things to improve:

  1. It looks like this block overflows its container.
image
  1. Disabled button should not have hover state (underline) and I would recommend to add disabled color (make opacity 0.8 for example or add custom disabled color)
image

return (
<footer className="todoapp__footer" data-cy="Footer">
<span className="todo-count" data-cy="TodosCounter">
{`${todos.filter(todo => !todo.completed).length} items left`}

Choose a reason for hiding this comment

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

It's better to extract this expression to a separate constant

<div
key={todo.id}
data-cy="Todo"
className={classNames('todo', { ' completed': todo.completed })}

Choose a reason for hiding this comment

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

Is this space here needed?

onSubmit={e => {
e.preventDefault();
updateTodo(todo.id);
}}

Choose a reason for hiding this comment

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

It's better to move these actions in a separate callback. Something like handleSubmit or handleUpdate

setEditing(null);
setNewTitle('');
}
}}

Choose a reason for hiding this comment

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

Same here. It's better to move these actions in a separate callback

className="todo__title"
onDoubleClick={() => {
setEditing(todo.id);
setNewTitle(todo.title);

Choose a reason for hiding this comment

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

Same here

@Denys-Kravchuk9988
Copy link

Good job!

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