-
Notifications
You must be signed in to change notification settings - Fork 0
react で todo app を作る #1
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ひとまずザッと見ましたー。
import CreateTodoForm from './components/CreateTodoForm'; | ||
import TodoList from './components/TodoList'; | ||
import * as TodosService from './local-storages/todos-service'; | ||
import useTodos from './hooks/use-todos'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
細かい突っ込みするとファイル名の命名ルールは統一したいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.tsx は PascalCase で
.ts は PascalCase 以外で
みたいなルールって一般的ではないですか?
vue の参考にしてたプロジェクトがそのようなルールっぽかったので踏襲してました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PascalCaseとkebabケースのファイル名が混在はあまり一般的ではないと思うよ!
いつくかパターンあるんだけど、パッといい参考リンクが出てこない、、(調べてみて!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.tsx は PascalCase で
.ts はcamelCase で
にしようと思います。
調べても良い記事が見つからなかったので、具体的なプロジェクトのパターンから習いました。
https://github.com/gothinkster/react-redux-realworld-example-app
google の styleguide だと以下のようです。
File names must be all lowercase and may include underscores (_) or dashes (-), but no additional punctuation. Follow the convention that your project uses. Filenames’ extension must be .js.
src/components/TodoList.tsx
Outdated
}; | ||
|
||
const handleClickEdit: HandleClickEdit = (todoId) => { | ||
setTodoBeingEdited({ ...todos.find((todo) => todo.id === todoId) } as Todo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このasの使い方だと何のためか読み解きにくいし、そもそも避けるのが難しいケース以外では気軽にasは使わないほうがいいというのが一般的にはよく言われる意見だと思います。
const selectedTodo = todos.find((todo) => todo.id === todoId)
if (!selectedTodo) throw Error('xxxx')
setTodoBeingEdited({...selectedTodo});
src/components/TodoList.tsx
Outdated
<div key={todo.id}> | ||
{isTodoBeingEdited(todo) ? ( | ||
<EditTodoForm | ||
todoTitleBeingEdited={todoBeingEdited!.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここも気軽に!
使わないほうがいいかなと。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自分ならprops名はシンプルにtitleとかvalueとか付けると思います。
今だと逆にどういう意味だろうと考えてしまってスッと読めないというか。
コンポーネントが適切に分かれていれば、そんなにややっこしい名前のpropsは生まれにくいはずです。(これくらいの規模なら)
src/components/TodoList.tsx
Outdated
}; | ||
|
||
const TodoList: FC<Props> = ({ todos, handleRemoveTodo, handleUpdateTodo }) => { | ||
const [todoBeingEdited, setTodoBeingEdited] = useState<Todo | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そもそも選択中のidだけじゃなくて選択中todoの情報を丸っと持つ必要があるのは何故なんだろう?
同じデータをあちこちで持つようになればなるほど状態管理が複雑になって後が辛くなりそうだなと。
src/components/TodoList.tsx
Outdated
|
||
// EditTodoForm の handler | ||
const handleChangeTodoTitleBeingEdited: HandleChangeTodoTitleBeingEdited = (event) => { | ||
setTodoBeingEdited((prevObj) => ({ ...prevObj, title: event.target.value } as Todo)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TodoListが状態として選択中todoの編集途中のtitleまで状態として持つ必要はないんじゃないかと思いました。(役割として不適切な気が)
編集途中のtitleはEditTodoFormだけが知っていればよくて、submit時にそのtitleが送られてくればそれで十分な気がするけどそんなことはないかな?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FormComponent を stateless にすべきかどうか悩んでました。
TodoList では編集中の todo の id だけ管理して、title の編集状態は EditTodoForm に持たせるようにします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そして場合によってはEditTodoFormでも入力途中のtitleを状態として持たないことも可能だなと思いました。(この辺は好みな気もするけど参考までに)
https://ja.reactjs.org/docs/uncontrolled-components.html
単にsubmit時に値が取れれば十分なら、submit時のeventから値を引っ張るという方法もあるなと。
const handleSubmit = (event) => {
const formData = new FormData(event.currentTarget)
xxxx(formData.get('title'))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどー
こういう書き方もあるんですね
メモメモ
レビューありがとうございました! |
Vue で todo app を作ったときに受けたフィードバックを活かしつつ、React でも todo app を作りました。
参考
https://qiita.com/soarflat/items/b9d3d17b8ab1f5dbfed2#%E3%81%AA%E3%81%9C-reactmemo-%E3%82%92%E5%88%A9%E7%94%A8%E3%81%99%E3%82%8B%E3%81%AE%E3%81%8B
https://tailwindcss.com/docs/extracting-components#extracting-component-classes-with-apply