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 1.1 #1674

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

Conversation

sborichevskyi
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.

Hi, it is bad practice to write all the code in one component, you need to separate the logic into different components, for example Header, Footer, TodoItem, TodoList

src/App.tsx Outdated
Comment on lines 29 to 30
client
.get<Todo[]>(`/todos?userId=${USER_ID}`)

Choose a reason for hiding this comment

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

Use method from api folder
image

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 improve your code

src/App.tsx Outdated
Comment on lines 27 to 28
client
.get<Todo[]>(`/todos?userId=${USER_ID}`)

Choose a reason for hiding this comment

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

Not fixed, use method getTodos() here
image

src/App.tsx Outdated
Comment on lines 29 to 40
.then(todosFromServer => {
setVisibleTodos(todosFromServer);
setLoading(false);
})
.catch(() => {
setError(true);
setErrorMessage('Unable to load todos');
setLoading(false);
})
.finally(() => {
setLoading(false);
});

Choose a reason for hiding this comment

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

Suggested change
.then(todosFromServer => {
setVisibleTodos(todosFromServer);
setLoading(false);
})
.catch(() => {
setError(true);
setErrorMessage('Unable to load todos');
setLoading(false);
})
.finally(() => {
setLoading(false);
});
.then(setVisibleTodos)
.catch(() => {
setError(true);
setErrorMessage('Unable to load todos')
})
.finally(() => setLoading(false));

return (
<footer className="todoapp__footer" data-cy="Footer">
<span className="todo-count" data-cy="TodosCounter">
{visibleTodos.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.

Move this logic from jsx to the helper varible and use it here

}

setSelectedFilter('all');
getTodos().then(allTodos => setVisibleTodos(allTodos));

Choose a reason for hiding this comment

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

Remove getTodos from here everywhere

<a
href="#/"
className={classNames('filter__link', {
selected: selectedFilter === 'all',

Choose a reason for hiding this comment

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

Create a enum for 'all', 'active', 'completed' and use it everywhere

Comment on lines 27 to 39
<a
href="#/"
className={classNames('filter__link', {
selected: selectedFilter === 'all',
})}
data-cy="FilterLinkAll"
onClick={() => {
if (selectedFilter === 'all') {
return;
}

setSelectedFilter('all');
getTodos().then(allTodos => setVisibleTodos(allTodos));

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

Comment on lines 46 to 54
onSubmit={event => {
event.preventDefault();
if (inputText.trim() === '') {
setError(true);
setErrorMessage('Title should not be empty');
} else {
createNewTodo(inputText, setError, setErrorMessage);
}
}}

Choose a reason for hiding this comment

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

Move this logic from jsx to the helper function

</label>

<span data-cy="TodoTitle" className="todo__title">
{todo.title}

Choose a reason for hiding this comment

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

Use destructuring for todo

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.

Almost done!
Let's make your code better

Comment on lines 45 to 61
switch (curFilter) {
case FilterEnum.ALL:
getTodos().then(allTodos => setVisibleTodos(allTodos));
break;
case FilterEnum.ACTIVE:
getActiveTodos().then(activeTodos =>
setVisibleTodos(activeTodos),
);
break;
case FilterEnum.COMPLETED:
getCompletedTodos().then(completedTodos =>
setVisibleTodos(completedTodos),
);
break;
default:
break;
}

Choose a reason for hiding this comment

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

Move this logic from jsx

src/api/todos.ts Outdated
Comment on lines 10 to 20
export const getCompletedTodos = () => {
return client
.get<Todo[]>(`/todos?userId=${USER_ID}`)
.then(allTodos => allTodos.filter(todo => todo.completed));
};

export const getActiveTodos = () => {
return client
.get<Todo[]>(`/todos?userId=${USER_ID}`)
.then(allTodos => allTodos.filter(todo => !todo.completed));
};

Choose a reason for hiding this comment

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

This logic is quite similar, filtering needs to be done on the front end so as not to make unnecessary requests.
image

@sborichevskyi
Copy link
Author

  • Moved Filtering logic from Footer.jsx to hepling function
  • Functions that filters todos dont do requests any more

Copy link

@vadiimvooo vadiimvooo left a comment

Choose a reason for hiding this comment

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

Well done

<section className="todoapp__main" data-cy="TodoList">
{visibleTodos.map(({ id, title, completed }) => {
return (
<div

Choose a reason for hiding this comment

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

Create separate component for Todo

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