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

solution #1587

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

solution #1587

wants to merge 5 commits into from

Conversation

RuslanMudryi
Copy link

Copy link

@andrushchenkoo andrushchenkoo 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!)

Copy link

@StasSokolov1 StasSokolov1 left a comment

Choose a reason for hiding this comment

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

Almost done, let's improve your solution a bit more!

src/App.tsx Outdated
try {
setLoadTodo({
id: 0,
title: todoQuery.trim(),

Choose a reason for hiding this comment

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

let's move todoQuery.trim() into const and reuse

Choose a reason for hiding this comment

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

still not fixed

src/api/todos.ts Outdated
};

export const addTodo = (todo: Omit<Todo, 'id' | 'userId' | 'completed'>) => {
return client.post<Todo>(`/todos`, {

Choose a reason for hiding this comment

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

move /todos into const and reuse

Choose a reason for hiding this comment

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

still not fixed

try {
setIsToLoad(true);

if (todoTitle.trim() === '') {

Choose a reason for hiding this comment

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

todoTitle.trim() move to variable and reuse

Comment on lines 122 to 130
onClick={async () => {
try {
setIsToLoad(true);
await onRemoveTodo(todo.id);
} catch (err) {
} finally {
setIsToLoad(false);
}
}}

Choose a reason for hiding this comment

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

move into function looks very complicated

Comment on lines 137 to 140
onSubmit={async e => {
e.preventDefault();
await saveChangesHandler();
}}

Choose a reason for hiding this comment

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

let's this also move into function

Copy link

@StasSokolov1 StasSokolov1 left a comment

Choose a reason for hiding this comment

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

Still not fixed some comments

src/App.tsx Outdated
try {
setLoadTodo({
id: 0,
title: todoQuery.trim(),

Choose a reason for hiding this comment

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

still not fixed

src/api/todos.ts Outdated
};

export const addTodo = (todo: Omit<Todo, 'id' | 'userId' | 'completed'>) => {
return client.post<Todo>(`/todos`, {

Choose a reason for hiding this comment

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

still not fixed

searchInput.current?.focus();
};

const ToggleAllHandler = async () => {

Choose a reason for hiding this comment

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

Suggested change
const ToggleAllHandler = async () => {
const toggleAllHandler = async () => {

Comment on lines 66 to 68
try {
await updateCompletedAllToDo();
} catch (err) {}

Choose a reason for hiding this comment

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

why we need try {} catch () {} with empty catch block?

Copy link

@StasSokolov1 StasSokolov1 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 to me, the comments are resolved!

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