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

Develop #27

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

Conversation

GitHubForDmitry
Copy link

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.

Позбудься масиву, можливо доведеться переробити логіку програми, але краще переробити ніж залишати все як є. Плюс почисти PR від зайвих файлів. Саме почисти. Коментарі пропасти не мають.


npm-debug.log*
yarn-debug.log*
yarn-error.log*
.idea
build
Copy link
Contributor

Choose a reason for hiding this comment

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

Навіщо мені всі ці файли?

@@ -0,0 +1,4 @@
module.exports = {
extends: "@mate-academy/stylelint-config",
rules: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

???

super();
this.state = {
todos: [],
filterTodos: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

через те що в тебе 2 масиви, в тебе є багато багів, які буде важко пофіксити. Якщо потестити твою todoApp то там є такі штуки: натискаємо check all, переходимо в вкладку completed і знімаємо прапорець checked all - результат, всі туду залишилися на місці, а items left  помінявся. Якщо checked all має бути активний тільки в тому випадку коли активні всі туду, тобто якщо я вручну знімаю прапорець з якоїсь туду він має ставати неактивним. І таких дрібниць багато. Наприклад якщо в тебе є виконані todo і перейти в вкладку active і натиснути clear completed то в вкладці active теж зникнуть todo, але вони будуть в all.
Моя порада - позбудься filterTodos і користуйся одним масивом

autoFocus=""
addTodo = (todo) => {
this.setState(prevState => ({
todos: [...prevState.todos, todo],
Copy link
Contributor

Choose a reason for hiding this comment

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

тобто ти її додав в todos а filtered про це нічого не знає. Це породжує нові баги, які можеш знайти сам, писати багато буде)


changeTodoCompleted = (id) => {
this.setState(prevState => ({
todos: prevState.todos.map(todo => (
Copy link
Contributor

Choose a reason for hiding this comment

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

знову ж таки змінив в одному масиві, а другий ні сном ні духом про це


destroyTodo = (id) => {
this.setState(prevState => ({
todos: prevState.todos.filter(todo => todo.id !== id),
Copy link
Contributor

Choose a reason for hiding this comment

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

аналогічно

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.

За файли зайві забудь, я зрозумів, то там з нашої сторони трабл якийсь

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