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 by Anna Kuvarina #15

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

Solution by Anna Kuvarina #15

wants to merge 9 commits into from

Conversation

AnnaKuvarina
Copy link

@AnnaKuvarina AnnaKuvarina commented Jul 26, 2019

Copy link

@vkryvytskyy vkryvytskyy left a comment

Choose a reason for hiding this comment

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

изображение

Сделай так чтобы нельзя было добавлять пустой todo

Screen Shot 2019-07-29 at 6 20 11 PM

Это число должно показывать количество еще не выполненных задач

src/App.js Outdated

handleChangeCompletedAll = () => {
this.setState(prevState => {
if (prevState.visibleTodos.every(todo => todo.completed === false)

Choose a reason for hiding this comment

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

every(todo => !todo.completed)

src/App.js Outdated
handleChangeCompletedAll = () => {
this.setState(prevState => {
if (prevState.visibleTodos.every(todo => todo.completed === false)
|| prevState.visibleTodos.every(todo => todo.completed === true)) {

Choose a reason for hiding this comment

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

every(todo => todo.completed)

src/TodoList.js Outdated
import './styles/todoList.css';

const TodoList = ({ todos, changeCompleted, changeCompletedAll, removeTodo}) => (
<section className="main" style={{ display: 'block' }}>

Choose a reason for hiding this comment

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

перенеси display: block в класс main в css файл

src/TodoList.js Outdated
{
todos.map(todo => {
const classes = classnames ({
'todo-active': todo.completed === false,

Choose a reason for hiding this comment

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

'todo-active': !todo.completed

src/TodoList.js Outdated
todos.map(todo => {
const classes = classnames ({
'todo-active': todo.completed === false,
'todo-completed': todo.completed === true,

Choose a reason for hiding this comment

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

'todo-completed': todo.completed

src/TodoList.js Outdated
type="checkbox"
name="completed"
className="toggle"
id={`todo-${todo.id}`}

Choose a reason for hiding this comment

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

префикс todo не делает id более уникальным

import propTypes from 'prop-types';

const TodosFilter = ({ handlerFilter }) => {

Copy link
Contributor

Choose a reason for hiding this comment

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

порада на майбутнє для таких речей створюєш масив цих фільтрів щось подібне:

[{name: 'all', text: 'All'}, {name: 'completed', text: 'Completed'}, ...]

І тоді не пишеш стільки коду при рендері, просто проходишся map по свому масиву і створєш список потрібних фільтрів, кількість коду менша, а змінити, щось буде набагато простіше, в одному місці міняєш, а міняється всюди)

src/App.js Outdated
...prevState.todos,
{
title,
id: prevState.todos.length + 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

тобто в тебе можуть todo з однакоми id? Якщо я видаляю першу тудушку то в нової доданої буде такий же id як і в передостанньої тудушки. Пофікси це

src/App.js Outdated

return {
visibleTodos: newTodos,
todos: newTodos,
Copy link
Contributor

Choose a reason for hiding this comment

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

Дійсно треба 2 копії зберігати? Може є спосіб як працювати тільки з одним масивом в один момент часу, а не тримати в пам'яті два ідентичні масиви?

Copy link
Contributor

Choose a reason for hiding this comment

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

Можеш не міняти, але на майбутнє думай про ефективне використання пам'яті

src/App.js Outdated
};
case 'active':
return {
visibleTodos: state.todos.filter(todo => (
Copy link
Contributor

Choose a reason for hiding this comment

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

state.todos.filter(todo => !todo.completed)

src/App.js Outdated
};
case 'completed':
return {
visibleTodos: state.todos.filter(todo => (
Copy link
Contributor

Choose a reason for hiding this comment

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

state.todos.filter(todo => todo.completed)

src/App.js Outdated
handlerClearCompleted = () => {
this.setState(prevState => {
const newTodos = prevState.visibleTodos.filter(todo => (
todo.completed === false
Copy link
Contributor

Choose a reason for hiding this comment

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

visibleTodos.filter(todo => !todo.completed)

});

return (
<li className={classes} key={todo.id}>
Copy link
Contributor

Choose a reason for hiding this comment

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

а ось це вже окремий компонент Todo

Copy link
Contributor

@vpolets vpolets left a comment

Choose a reason for hiding this comment

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

при додаванні нової todo в мене пропадає все, просто пусте вікно стає. Перевір чи в тебе все ок

Copy link
Contributor

@vpolets vpolets left a comment

Choose a reason for hiding this comment

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

Молодчина)

</section>
);

<footer className="footer" style={{ display: 'block' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

по-хорошому Footer це окремий компонент

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