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

Feature/create-update-todo-process #29

Merged
merged 17 commits into from Mar 13, 2019

Conversation

@ha-matsumu
Copy link
Owner

ha-matsumu commented Mar 11, 2019

  • Todoをクリックした時にフォームにTodoの内容を表示する処理

  • 選択されたTodoを更新する処理

上記2点を作成しました。レビューよろしくお願いいたします。

今後別ブランチでFormをmodal的に表示させようと思っているので、その時にCancelボタンの処理を作成しようと考えています。

handleInputChange(event) {
componentDidUpdate() {
if (this.props.selectedTodoId !== this.state.id) {
axios.get("/api/todos/" + this.props.selectedTodoId).then(response => {

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

ここのaxiosの処理はReduxのactionsとreducer側で実装するべきかなと思うのですがどうでしょうか。
実装した方がいいなら、別ブランチで作成しようと思います。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

actions側にTodo APIの実行をまとめて、それを利用するようにしたほうが良いかなと思います。


https://github.com/ha-matsumu/todo-with-express-and-mysql/pull/29/files#diff-cc4fdb3266cfd3c6c6d3328cfbebe43fR2

connect を使って、他のコンテナーと同じように、 mapDispatchToProps にactions経由でAPIを実行する実装をすると良いかなと思います。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

別ブランチで作成します。

</select>
</label>
)}
<button className="Cancel" onClick={this.cancelHandler}>Cancel</button>

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

ボタンはコンポネート化するべきかなと思っています。こちらも別のブランチで作成しようと考えています。

This comment has been minimized.

Copy link
@tsuyopon-xyz

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

別ブランチで作成します。

@@ -4,7 +4,7 @@ import PropTypes from "prop-types";
import "./Todo.css";

const Todo = props => (
<article className="todo">
<article className="todo" onClick={props.clicked}>

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

props.clicked は新しく追加したpropsかと思うので、 Todo.propTypes clicked の追加をお願いしますmm


また、clicked という名前ですが、関数というよりもbool値だと勘違いされる可能性もあるかなと思いました。(実際に completed はbool値であるため。)

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

こちら article要素に対して onClick セットしていますが、以下のように直接Componentにセットできなかったでしたっけ?(僕自身も記憶が曖昧です^^;)

<Todo id="..." onClick={() => {}}>

https://github.com/ha-matsumu/todo-with-express-and-mysql/pull/29/files#diff-289d0f56d33d0b50a055ac1555d45f97R41

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

props.clicked は新しく追加したpropsかと思うので、 Todo.propTypes clicked の追加

追加しました。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

clickedという名前ですが、関数というよりもbool値だと勘違いされる可能性

名前変更しました。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

直接Componentにセットできなかったでしたっけ?

<Todo id="..." onClick={() => this.selectTodoHandler(todo.id)} >
上記の方法でonClickをセットしたのですが、動かなくなりました。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

<Todo id="..." onClick={() => {}}> はダメなんですね^^;
了解しました^^

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

また、パフォーマンスの関係であまりアロー関数の書き方をオススメしないという記述も見られました。

componentDidUpdate() {
if (this.props.selectedTodoId !== this.state.id) {
axios.get("/api/todos/" + this.props.selectedTodoId).then(response => {
this.setState({

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

以下の記事を見ると componentDidUpdate 内で setState を実行してはいけないようなことが書かれていますが、問題は発生していないでしょうか?(無限ループが起きるみたいなことが書かれています。)

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

https://reactjs.org/docs/react-component.html#componentdidupdate

You may call setState() immediately in componentDidUpdate() but note that it must be wrapped in a condition like in the example above, or you’ll cause an infinite loop.

ここに書いてある例のような条件でラップしていれば無限ループを避けられるみたいです。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

なるほど if (this.props.selectedTodoId !== this.state.id) が最初はtrueなのが、更新されると、idが同じ値になりfalseになるということですね^^

</div>
);
}
}

TodoForm.propTypes = {
addTodo: PropTypes.func.isRequired
addTodo: PropTypes.func.isRequired,
updateTodo: PropTypes.func.isRequired

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

以下のコードを見ると selectedTodoId もpropsとして渡しているかと思うので、その記述もお願いします

https://github.com/ha-matsumu/todo-with-express-and-mysql/pull/29/files#diff-289d0f56d33d0b50a055ac1555d45f97R58

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

修正しました。

this.props.addTodo(todo);
{
this.state.requestAdd
? this.props.addTodo(todo)

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

this.props.addTodo(todo)this.props.updateTodo(todo); のいずれも非同期メソッドになるかと思いますが、非同期処理が完了する前に、直後のthis.setStateが実行されますが、問題ないでしょうか?

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

現状問題なさそうです。

title: "",
body: "",
completed: false
completed: false,
requestAdd: true

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

requestAdd は新規作成APIを実行するか、更新APIを実行するかのフラグを表しているのかなと思いますが、認識あってますでしょうか?


もし合っていれば、フラグをtrueにするかfalseにするかは selectedTodoId の有無によるのかなと思いますが、認識あってますでしょうか?


もし合っていれば、

requestAdd: true` 

の部分を

requestAdd: props.selectedTodoId ? false : true

もしくは、

requestAdd: !props.selectedTodoId, // IDが渡されてたら更新APIを実行するためのフラグをセット

のようにすることも出来るかなと思いました。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

requestAdd は新規作成APIを実行するか、更新APIを実行するかのフラグを表しているのかなと思いますが、認識あってますでしょうか

もし合っていれば、フラグをtrueにするかfalseにするかは selectedTodoId の有無によるのかなと思いますが、認識あってますでしょうか?

tsuyoponさんがおっしゃられている通りです。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

のようにすることも出来るかなと思いました。

requestAdd: !props.selectedTodoId,の書き方に変更してみたのですが、
Todoの更新時に

UpdateTodo

Update Todoを押した後FormAdd Todoの状態

AddTodo

に戻したいのですが戻らなくなってしまいます。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

もとの書き方でも動画のように「Addの時のフォーム」が自動的に「updateの時のフォーム」に変わってしまいます。
demo mov
現在ここの修正方法を模索しています。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

シンプルに考えられる方法としては、新規作成用と更新用のTodoFormを2つ用意して、

TodoFormを利用する TodoList コンポーネントで条件に応じてコンポーネントを切り替えるっていうのが1つの手かなと思いました。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

Update Todoを押した後FormをAdd Todoの状態
に戻したいのですが戻らなくなってしまいます。

#29 (comment)

上記に関することですが、片方は非同期処理が完了してからthis.setState を実行していて、片方は非同期処理の完了を待たずに this.setState のように、setState がいろんなタイミングで呼ばれるのが怪しいかなと考えています。

つまり、言いたいことは setState が実行されるタイミングを意図通りのタイミングで実行されるようにした方が良いということです。

呼ばれるsetStateが呼ばれるタイミングがバラバラということは、setStateによって更新される表示内容が非同期処理のタイミングによってバラバラになる可能性があるということも意味しているため。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

シンプルに考えられる方法としては、新規作成用と更新用のTodoFormを2つ用意

Formを2つ用意して対応しました。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

#29 (comment)

片方は非同期処理が完了してからthis.setState を実行していて、片方は非同期処理の完了を待たずに this.setState のように、setState がいろんなタイミングで呼ばれるのが怪しいかなと考えています。

申し訳ございません。非同期処理の学習不足なのかこの辺り原因がわからず、現状修正できていないです。コードの流れもう一度追ってみて調べてみます。

title: this.state.title,
body: this.state.body,
completed: this.state.completed
};
this.props.addTodo(todo);
{

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

ブロック({})で囲っているのは何か意図があるのでしょうか?

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

意味のないブロックでしたので削除しました。

@@ -1,53 +1,82 @@
import React, { Component } from "react";
import { connect } from "react-redux";

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 11, 2019

connect をこのファイル内で使っていないような気がするのですがいかがでしょうか?

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 11, 2019

Author Owner

削除しました。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Mar 11, 2019

いくつかコメントしたのでご確認お願いしますmm

@ha-matsumu

This comment has been minimized.

Copy link
Owner Author

ha-matsumu commented Mar 12, 2019

コード修正しました。レビューよろしくお願いいたします。

if (!this.state.title) return;
const todo = {
id: this.state.id,

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

ここの id は不要な気がするのですが如何でしょうか?

  • 新規作成であれば DBで自動採番される
  • 更新であれば、 /api/todos/:id のようにURLの :id にセットされる

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

更新であれば、 /api/todos/:id のようにURLの :id にセットされる

/api/todos/:id:id${todo.id}でidをセットしているので必要かと思ったのですがどうでしょうか?

const response = await axios.put(`/api/todos/${todo.id}`, todo);

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

あ、なるほど^^;
すみません、以下のコードとごっちゃに考えていましたmm

https://github.com/ha-matsumu/todo-with-express-and-mysql/pull/29/files#diff-d6abe09095970c8ff1da6f82bcdedc16R23

@@ -44,7 +44,7 @@ class TodoList extends Component {
title={todo.title}
body={todo.body}
completed={todo.completed}
clicked={() => this.selectTodoHandler(todo.id)}
selectedTodo={this.selectTodoHandler.bind(this, todo.id)}

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

selectedTodo も関数名というよりは、 選択したTodoのオブジェクトが入っているような印象を持ちます。

関数名は基本的に 現在形の動詞 + 目的語 の形式が一般的かと思います。
もしくは、今回の場合クリックしたときに指定した処理を実行したいのであれば、 onClick のようなスタイルで、 onClickTodo, onSelectingTodo の方が、関数がセットされているのかなと認識されやすいです。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

現在形の動詞 + 目的語の形式に修正しました。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Mar 12, 2019

コメント&返答したのでご確認お願いしますmm

@ha-matsumu

This comment has been minimized.

Copy link
Owner Author

ha-matsumu commented Mar 12, 2019

コードの修正・返答しました。ご確認よろしくお願いいたします。

body: this.state.body,
completed: this.state.completed
};
this.props.addTodo(todo);

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

#29 (comment)

ここが本来なら、処理が一通り完了してから setState を更新した方が良いので、 async/awaitを使うなら await this.props.addTodo(todo); とするべきになります。

そうすることで、非同期処理が完了してから、つまりawaitでPromiseがresolveの状態になってから次の処理にいくようになります。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

実際にはサーバーの問題で this.props.addTodo(todo); はサーバーエラーやDBエラーなどで失敗する可能性があるので、その際にはフォームの中身はそのままにエラーになったことを知らせるのがユーザーに優しいです。
(そうしないと失敗した際にまたデータを送信したいのに、またゼロから入力をすることになるからです。)

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

#29 (comment)

Web白熱教室で非同期処理を勉強し直し理解できました。

This comment has been minimized.

Copy link
@ha-matsumu

ha-matsumu Mar 12, 2019

Author Owner

実際にはサーバーの問題で this.props.addTodo(todo); はサーバーエラーやDBエラーなどで失敗する可能性があるので、その際にはフォームの中身はそのままにエラーになったことを知らせるのがユーザーに優しいです

フォームの中身を残したまま表示するには、

const requestError = error => {
を修正するか、
を修正する必要があると思うのですがどうでしょうか?
ここの作成は別ブランチで行おうと思います。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Mar 12, 2019

を修正する必要があると思うのですがどうでしょうか?

イメージ的には、TodoFormのstateを更新せずに保持し続ければよいのかなと思いますが、 reducerで管理しているerrorオブジェクトが更新されて、 TodoList.js のrenderメソッドが再実行されたときに、仮に、同じAddFormを使い続けていたとしても、AddFormのstateがリセットされるのかどうかが気になるところです。

https://github.com/ha-matsumu/todo-with-express-and-mysql/pull/29/files#diff-289d0f56d33d0b50a055ac1555d45f97R31


もしresetされるのであれば、TodoFormのstateの値をどうにかして、保持し続けるような実装を考える必要があります。

ここの作成は別ブランチで行おうと思います。

👍

body: this.state.body,
completed: this.state.completed
};
this.props.updateTodo(todo);

This comment has been minimized.

Copy link
@tsuyopon-xyz

This comment has been minimized.

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Mar 12, 2019

コメントしたのでご確認お願いしますmm

@ha-matsumu

This comment has been minimized.

Copy link
Owner Author

ha-matsumu commented Mar 12, 2019

コードの修正・返答しました。ご確認よろしくお願いいたします。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Mar 12, 2019

返答しました&一旦このプルリクでの実装内容はマージしてもOKです^^


とりあえず、別ブランチで行うとコメントしたものを後からすぐに見返せるように issues などにまとめておくと良いかなと思います。

そして、新しくプルリクを作成するときは、そのプルリクがどの issue に対応するプルリクなのか、 issue のリンクを貼っておくと良いです。

そうすること、プルリクがマージされたときに、そのissueも解決したことになるので、リンクから飛んで、issueをcloseに出来るからです。

よろしくお願いしますmm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.