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.

Sign up for GitHub

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

Jump to bottom

finalize solution with some skipped tests #822

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

finalize solution with some skipped tests #822

wants to merge 5 commits into from

Conversation

serkrops
Copy link

Vayts
Vayts approved these changes Sep 28, 2023
View reviewed changes
Copy link

@Vayts Vayts left a comment

Choose a reason for hiding this comment

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

lgtm!

Moroz-Dmytro
Moroz-Dmytro suggested changes Sep 29, 2023
View reviewed changes
Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

For the future, give more descriptive commit names)

image

src/App.tsx Outdated
Comment on lines 22 to 23
const [activeTodosCount, setActiveTodosCount] = useState<number>(0);
const [isAnyTodoCompleted, setIsAnyTodoCompleted] = useState(false);
Copy link

Choose a reason for hiding this comment

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

Do you really need state for this, can it be just constants?

src/App.tsx Outdated
Comment on lines 109 to 112
id: todo.id,
title: newTodoTitle,
userId: todo.userId,
completed: todo.completed,
Copy link

Choose a reason for hiding this comment

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

Suggested change
id: todo.id,
title: newTodoTitle,
userId: todo.userId,
completed: todo.completed,
...todo,
title: newTodoTitle,

src/App.tsx Outdated
Comment on lines 139 to 143
setTodos(prevState => prevState.map(currentTodo => (
currentTodo.id !== updatedTodo.id
? currentTodo
: updatedTodo
)));
Copy link

Choose a reason for hiding this comment

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

This logic repeats some times, maybe let's move this to separate function

src/App.tsx Outdated
)}
</section>

{/* Hide the footer if there are no todos */}
Copy link

Choose a reason for hiding this comment

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

Don't leave comments in code

src/App.tsx Outdated
</section>

{/* Hide the footer if there are no todos */}
{(todos.length !== 0) && (
Copy link

Choose a reason for hiding this comment

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

Suggested change
{(todos.length !== 0) && (
{!!todos.length && (

src/App.tsx Outdated
Comment on lines 219 to 220
{/* Notification is shown in case of any error */}
{/* Add the 'hidden' class to hide the message smoothly */}
Copy link

Choose a reason for hiding this comment

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

Delete comments

src/App.tsx Outdated
Comment on lines 221 to 242
<div
data-cy="ErrorNotification"
className={classNames(
'notification',
'is-danger',
'is-light',
'has-text-weight-normal',
{
hidden: !errorMessage,
},
)}
>
<button
data-cy="HideErrorButton"
type="button"
className="delete"
onClick={() => {
setErrorMessage('');
}}
/>
{errorMessage}
</div>
Copy link

Choose a reason for hiding this comment

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

Move this to separate component

src/Components/TodoFooter.tsx Outdated
))}
</nav>

{/* don't show this button if there are no completed todos */}
Copy link

Choose a reason for hiding this comment

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

Delete

Moroz-Dmytro
Moroz-Dmytro approved these changes Sep 29, 2023
View reviewed changes
Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

LGTM 👍
But check some comments below

src/App.tsx
Comment on lines +38 to +39
const activeTodosCount = todos.filter(todo => todo.completed !== true).length;
const isAnyTodoCompleted = todos.some(todo => todo.completed === true);
Copy link

Choose a reason for hiding this comment

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

Suggested change
const activeTodosCount = todos.filter(todo => todo.completed !== true).length;
const isAnyTodoCompleted = todos.some(todo => todo.completed === true);
const activeTodosCount = todos.filter(({ completed })=> !completed).length;
const isAnyTodoCompleted = todos.some(({ completed }) => completed);

src/App.tsx
});
};

const handleRenameTodo = (todo: Todo, newTodoTitle: string) => {
Copy link

Choose a reason for hiding this comment

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

This function and handleToggleTodo are very similar, we can combine them

src/App.tsx
Comment on lines +208 to +210

</div>
<ErrorMessage
Copy link

Choose a reason for hiding this comment

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

Suggested change
</div>
<ErrorMessage
</div>
<ErrorMessage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Moroz-Dmytro Moroz-Dmytro Moroz-Dmytro approved these changes

@Vayts Vayts Vayts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serkrops @Vayts @Moroz-Dmytro