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 #777

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

Solution #777

wants to merge 30 commits into from

Conversation

HaniaNassalska
Copy link

Copy link

@choeqq choeqq 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, please try to create generic Button component, where you will pass some props like onClick and text to render and use this kind of component throughout the app - you won't have to create a component for every single button, you will only pass props to it

Copy link

Choose a reason for hiding this comment

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

when ToggleButton is pressed, every single todo starts loading, please try to fix it

Copy link
Author

Choose a reason for hiding this comment

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

I checked it and it works. Could you show me a case where it doesn't work properly?
https://monosnap.com/file/cSfxJXozDR8dt1FAI7TECDTZxiEHFj

Copy link

Choose a reason for hiding this comment

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

this directory should probably be named context, and the file should be a TodoContext.tsx

import { Task } from './Task';
import { useTodo } from '../../provider/todoProvider';

export const TaskList = () => {
Copy link

Choose a reason for hiding this comment

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

since we're using Todo name throughout the app, I would stick to it and name this component TodoList

Comment on lines 11 to 21
const visibleTodos = () => {
if (filterTodos === 'active') {
return todos.filter(todo => !todo.completed);
}

if (filterTodos === 'completed') {
return todos.filter(todo => todo.completed);
}

return todos;
};
Copy link

Choose a reason for hiding this comment

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

this can be a util function


return (
<header className="todoapp__header">
{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

const loaderCases
= ((temptTodo && temptTodo.id === todo.id)
|| (editedTodo && todo.completed)
|| todo.hasLoader) as boolean;
Copy link

Choose a reason for hiding this comment

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

why do you need as boolean here?

className="todoapp__main"
data-cy="TodoList"
>
{visibleTodos().map(todo => (
Copy link

Choose a reason for hiding this comment

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

visibleTodos should be value, instead of function

Comment on lines 92 to 94
setTodos((prevState) => {
return prevState.filter(t => t.id !== task.id);
});
Copy link

Choose a reason for hiding this comment

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

Suggested change
setTodos((prevState) => {
return prevState.filter(t => t.id !== task.id);
});
setTodos((prevState) => prevState.filter(t => t.id !== task.id));

Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

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

LGTM ✅

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