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

ToDoのstate管理方法の変更 #17

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kacha-122
Copy link
Collaborator

@kacha-122 kacha-122 commented Oct 9, 2021

拡張性をあげるため

🔨 Details of Changes

  • type TodoTypeidisCompletedパラメーターを追加
  • initialTodosidisCompletedのデータを追加
  • 関連する箇所の細かな修正

✅ Resolved Issues

🤝 Related Issues

@kacha-122
Copy link
Collaborator Author

明日の午後からおそらく再開しますが、夕方までわからなかったらディスコードで泣きつくと思うので、お手すきの方は来ていただけると助かります。

kahca-122 added 2 commits October 10, 2021 18:50
データの管理方法を変更するため
それぞれのタスクでidを管理するように変更するため
const newStatus = [...status];
newStatus[index] = !newStatus[index];
setStatus(newStatus);
const newStatus = [...todos];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const newStatus = todos;
上記の書き方だとうまく動作しませんでしたが、理由はあんまりわかってないです。

Choose a reason for hiding this comment

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

自分も原因分からないです...。
const をいじってるのが怪しい?とも思ったけど、変更は配列の中身なので問題ないし。
あえて言うなら、基本 const で定義しているように、React のスタイル??は一度定義した変数を書き換えて使い回すのではなく、どんどん変更がある度に新しい変数に放り込んでいくので、そういう実装にしてみても良いかもしれません。
と言ってもここの場合だと、やるとしたらこんな感じ?であまり良い例&実装ではありませんが。

const handleStatus = () => {
  const newStatus = todos.map((todo: TodoType, i: number): TodoType => {
    return i === index
      ? { id: todo.id, title: todo.title, isCompleted: !todo.isCompleted }
      : todo;
  });
  setTodos(newStatus);
};

Copy link
Contributor

@watagit watagit Oct 10, 2021

Choose a reason for hiding this comment

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

ちょっと調べた感じだと、一般的にはslice()を使うみたいです(逆に今まで私どうやってたんだって感じですが)

[Qiita] ReactのsetStateで配列の一部を変更する
[Qiita]【React】Reactで便利な配列操作まとめ

Copy link

@mikiya1130 mikiya1130 Oct 10, 2021

Choose a reason for hiding this comment

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

じゃあやっぱここはコピーが必要なのかな。
[...status]status.slice() には何か違いあるのかな?
コピーの深さも同じ?気がするけど。
status.slice() の方が引数で範囲指定できる点で上位互換ってことかな。

@kacha-122 kacha-122 marked this pull request as ready for review October 10, 2021 11:00
Copy link

@mikiya1130 mikiya1130 left a comment

Choose a reason for hiding this comment

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

お疲れ様です。
全体的にはとても良くなったと思います。
自分からは1点だけ、よろしくお願いいたします。

const newStatus = [...status, false];
const newTodos = [
...todos,
{ id: String(todos.length), title: title, isCompleted: false },
Copy link

@mikiya1130 mikiya1130 Oct 10, 2021

Choose a reason for hiding this comment

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

must
todos.length から map の key で使う id を生成すると、id が重複することがあります 😢
(例:末尾以外の Todo を削除 → Todo を新規追加)
タスク追加ごとにひたすらインクリメントのみしてく値を用意するなど、他に対策が必要そうです。
map の key 専用で、なんでもいいなら作成時のタイムスタンプや、場合によっては乱数でも問題ないかもだけど。

(動画は useEffect 追加してます。)

2021-10-10.20-55-36.mp4

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
Collaborator Author

Choose a reason for hiding this comment

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

今回は「タスク追加ごとにひたすらインクリメントのみしてく値を用意する」というもので解消しました。
ご指摘ありがとうございました!

IDの設定方法を変更するため
@kacha-122 kacha-122 requested a review from mikiya1130 October 12, 2021 20:53
@watagit watagit removed their request for review October 20, 2021 07:03
type TodoType = {
title: string;
};
import TodoInput from '@/conponents/TodoInput';
Copy link

Choose a reason for hiding this comment

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

素敵です!

@@ -1,27 +1,23 @@
import { Container } from '@chakra-ui/react';
import React, { useState } from 'react';

Choose a reason for hiding this comment

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

良いコードですね!

@mikiya1130 mikiya1130 self-requested a review February 10, 2023 05:05
@@ -1,27 +1,23 @@
import { Container } from '@chakra-ui/react';
import React, { useState } from 'react';

import TodoInput from './conponents/TodoInput';

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.

Todo の state 管理の変更 タスクの削除が正しく機能しない
4 participants