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

commited unworking code #804

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

Conversation

OPTIMISTIXX
Copy link

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Unfortunately, your project is not reachable via the demo link(

Comment on lines 6 to 7
removeItem,
markAsCompleted

Choose a reason for hiding this comment

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

u don't use these 2 functions in this component

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Pls watch the video review via the link :)

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

try to fix this difference in height when user tries to edit the todo
https://github.com/mate-academy/react_todo-app/assets/70220667/e87259cc-b553-4ed8-8eb8-a2df19c16ff5

Comment on lines 16 to 47
{filter === 'all' && (
todos
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}
{filter === 'active' && (
todos
.filter((todoItem) => todoItem.completed === false)
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}
{filter === 'completed' && (
todos
.filter((todoItem) => todoItem.completed === true)
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}

Choose a reason for hiding this comment

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

looks too complex, just filter the array before the jsx, save in some variable and map in return


export const Footer = () => {
const { state: { todos }, dispatch } = React.useContext(TodosContext);
const [state, setState] = useState(todos);

Choose a reason for hiding this comment

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

it can be the simple variable instead of the state

src/App.tsx Outdated
<input type="text" className="edit" />
</li>
const { state: { todos } } = React.useContext(TodosContext);
const [tasksLength, setTasksLength] = useState(todos.length);

Choose a reason for hiding this comment

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

same here

Copy link

@sTorba24 sTorba24 left a comment

Choose a reason for hiding this comment

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

  1. remove this dot
    image

  2. show delete button for all items, it's not clear that i should hover to see deletion

Comment on lines 10 to 14
if (labelClick % 2 === 0) {
dispatch({ type: 'MARK_ALL_AS_UNCOMPLETED' });
} else {
dispatch({ type: 'MARK_ALL_AS_COMPLETED' });
}

Choose a reason for hiding this comment

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

Suggested change
if (labelClick % 2 === 0) {
dispatch({ type: 'MARK_ALL_AS_UNCOMPLETED' });
} else {
dispatch({ type: 'MARK_ALL_AS_COMPLETED' });
}
dispatch({ type: !(labelClick % 2)
? 'MARK_ALL_AS_UNCOMPLETED'
: 'MARK_ALL_AS_COMPLETED'
});

Comment on lines 17 to 37
if (!todos.length) {
return null;
}

return (
<section className="main">
<input
onClick={handleToggleAll}
type="checkbox"
id="toggle-all"
className="toggle-all"
data-cy="toggleAll"
/>
<label
htmlFor="toggle-all"
>
Mark all as completed
</label>
<Todos />
</section>
);

Choose a reason for hiding this comment

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

Suggested change
if (!todos.length) {
return null;
}
return (
<section className="main">
<input
onClick={handleToggleAll}
type="checkbox"
id="toggle-all"
className="toggle-all"
data-cy="toggleAll"
/>
<label
htmlFor="toggle-all"
>
Mark all as completed
</label>
<Todos />
</section>
);
return (
{ todos.length > 0 && (
<section className="main">
<input
onClick={handleToggleAll}
type="checkbox"
id="toggle-all"
className="toggle-all"
data-cy="toggleAll"
/>
<label
htmlFor="toggle-all"
>
Mark all as completed
</label>
<Todos />
</section>
)
}
);

Choose a reason for hiding this comment

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

not fixed

Comment on lines 19 to 53
return (
<ul className="todo-list" data-cy="todoList">
{filter === 'all' && (
todos
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}
{filter === 'active' && (
activeFilter
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}
{filter === 'completed' && (
complietedFilter
.map((todoItem) => (
<Todo
key={todoItem.id}
todoItem={todoItem}
onItemDelete={handleDelete}
/>
))
)}

</ul>
);

Choose a reason for hiding this comment

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

prepare filtered todos before render just, and then use only variable filteredTodos, now logic is too complicated

Choose a reason for hiding this comment

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

also create an enum for filter types

@OPTIMISTIXX OPTIMISTIXX requested a review from sTorba24 December 26, 2023 11:28
Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Pls fix prev comments
Screenshot at Dec 26 14-07-03

Comment on lines 17 to 37
if (!todos.length) {
return null;
}

return (
<section className="main">
<input
onClick={handleToggleAll}
type="checkbox"
id="toggle-all"
className="toggle-all"
data-cy="toggleAll"
/>
<label
htmlFor="toggle-all"
>
Mark all as completed
</label>
<Todos />
</section>
);

Choose a reason for hiding this comment

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

not fixed

Copy link

@sTorba24 sTorba24 left a comment

Choose a reason for hiding this comment

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

I approve it, but still not clear that i'm able to remove 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.

4 participants