Skip to content

composition-api を用いて 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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Jan 10, 2021
Merged

composition-api を用いて todo-app を作ってみる #1

merged 19 commits into from
Jan 10, 2021

Conversation

r-toki
Copy link
Owner

@r-toki r-toki commented Dec 25, 2020 •
edited
Loading

  • vue3 & ts で書いた
  • vuex っぽい？ store を composition-api で作った。composables/use-todos-store.ts
  • component を分けた。名前が変かも。違和感あったら教えてください
    • NewTodoForm.vue
    • EditTodoForm.vue
    • TodoItem
  • アイコンは font-awesome を使った
  • localStorage へのアクセスは storage/todos-service.ts 経由で

おすすめリンク

おすすめ

  • ESLint の設定は plugin:vue/vue3-recommended に

@aki77
Copy link

aki77 commented Dec 25, 2020

https://v3.vuejs.org/guide/composition-api-introduction.html

composition apiの書き方はこの辺が参考になりそう。

aki77
aki77 reviewed Dec 25, 2020
View reviewed changes
aki77
aki77 reviewed Dec 25, 2020
View reviewed changes
aki77
aki77 reviewed Dec 25, 2020
View reviewed changes
aki77
aki77 reviewed Dec 25, 2020
View reviewed changes
aki77
aki77 reviewed Dec 27, 2020
View reviewed changes
src/libs/FontAwesomeIcon.vue Outdated
<template>
<svg
xmlns="http://www.w3.org/2000/svg"
:class="$props.class"
Copy link

Choose a reason for hiding this comment

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

変更漏れかな？

Copy link
Owner Author

Choose a reason for hiding this comment

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

ここ参考に font-awesome を導入しました。
FortAwesome/vue-fontawesome#230 (comment)

Copy link

Choose a reason for hiding this comment

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

↓で問題なさそうに見えるけど駄目でした？

:class="class"

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど！

Copy link
Owner Author

Choose a reason for hiding this comment

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

:class="class" にするとエラー出てきます。

 error  in ./src/libs/FontAwesomeIcon.vue

Module Error (from ./node_modules/eslint-loader/index.js):

/Users/ryosuke/Repos/todo-app-vue3/src/libs/FontAwesomeIcon.vue
  4:18  error  Parsing error: Unexpected end of expression  vue/no-parsing-error

✖ 1 problem (1 error, 0 warnings)

Copy link

Choose a reason for hiding this comment

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

何でだろ。
時間あるとき実際に動かして見てみよう。

Copy link

Choose a reason for hiding this comment

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

多分classって名前が悪い。
別の名前にしちゃえば行けそう！

Copy link
Owner Author

Choose a reason for hiding this comment

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

別名にしたらエラー解消しました！

@aki77
Copy link

aki77 commented Dec 27, 2020

あとeslintのルールは、少なくても慣れるまではplugin:vue/vue3-essentialではなくてplugin:vue/vue3-recommendedとするほうがオススメです

aki77
aki77 reviewed Dec 27, 2020
View reviewed changes
aki77
aki77 reviewed Dec 27, 2020
View reviewed changes
aki77
aki77 reviewed Dec 27, 2020
View reviewed changes
aki77
aki77 reviewed Dec 27, 2020
View reviewed changes
aki77
aki77 reviewed Dec 29, 2020
View reviewed changes
aki77
aki77 reviewed Dec 29, 2020
View reviewed changes
aki77
aki77 reviewed Dec 29, 2020
View reviewed changes
aki77
aki77 reviewed Dec 30, 2020
View reviewed changes
@aki77
Copy link

aki77 commented Dec 30, 2020

一応補足

指摘が100%正解なんてことはないし、全部修正してほしいと思ってコメントしている訳でもないので、そうは言うけどおかしくない？とか今の方が良くない？とかあれば遠慮なく聞いてくださいー。

@r-toki
Copy link
Owner Author

r-toki commented Dec 30, 2020 •
edited
Loading

今は上級者の感覚に近づきたく、指摘箇所は全て修正しようと思っています。
一応指摘はどれも納得してから修正しているつもりです。（似たようなミス多いですがw）

@aki77
Copy link

aki77 commented Dec 31, 2020

あけおめです！大体質問には返事したつもりだけど、もしも返事漏れあったら教えて下さい。

@r-toki r-toki closed this Jan 3, 2021
@r-toki r-toki reopened this Jan 3, 2021
aki77
aki77 reviewed Jan 3, 2021
View reviewed changes
src/components/CreateTodoForm.vue Outdated
state.title = newValue;
}
);
watch(
Copy link

Choose a reason for hiding this comment

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

あんまりwatch多用するとコード追いにくくなるので、普通にeventで処理できるところではイベント使う派です。（ここでは@input使う）

src/components/TodoItem.vue Outdated

export type HandleToggleCompleted = (todo: Todo) => void;
export type HandleClickEdit = (todo: Todo) => void;
export type HandleClickRemove = (todo: Todo) => void;
Copy link

Choose a reason for hiding this comment

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

好み？

自分ならHandleClickEditとHandleClickRemoveの引数はidだけにするかも。
そっちの方が変更に強そうというか。

src/components/TodoItem.vue Outdated
export type HandleClickRemove = (todo: Todo) => void;

export default defineComponent({
components: { fa: FontAwesomeIcon },
Copy link

Choose a reason for hiding this comment

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

好み

自分なら↓みたいに書いて、コンポーネントと通常のhtmlタグを視覚的にも区別しやすくします。
あとそっちのほうがgrepしやすい気もするけど、実際はそんなに大差ないかも。

Suggested change
components: { fa: FontAwesomeIcon },
components: { FontAwesomeIcon }, 
<FontAwesomeIcon icon="trash" />

Copy link

Choose a reason for hiding this comment

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

kebabケースで書く場合は、ハイフン入りの名前が推奨と公式ガイドにあったはずです。

https://jp.vuejs.org/v2/style-guide/index.html#%E3%83%86%E3%83%B3%E3%83%97%E3%83%AC%E3%83%BC%E3%83%88%E5%86%85%E3%81%A7%E3%81%AE%E3%82%B3%E3%83%B3%E3%83%9D%E3%83%BC%E3%83%8D%E3%83%B3%E3%83%88%E5%90%8D%E3%81%AE%E5%BD%A2%E5%BC%8F-%E5%BC%B7%E3%81%8F%E6%8E%A8%E5%A5%A8

Copy link
Owner Author

Choose a reason for hiding this comment

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

修正しました。

src/components/TodoList.vue Outdated
export type HandleSubmitEditedTodo = (editedTodo: Todo) => void;

type State = {
todoIdBeingEdited: string | null;
Copy link

Choose a reason for hiding this comment

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

好み

tsでの書きやすさ考えると、こういうのはnullではなくundefined使う派です。

Suggested change
todoIdBeingEdited: string | null;
todoIdBeingEdited?: string;

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど！

修正しました。

src/pages/Index.vue Outdated
import type {
HandleToggleCompleted,
HandleClickRemove,
} from "@/components/TodoItem.vue";
Copy link

Choose a reason for hiding this comment

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

細かいこと

このコンポーネントが孫コンポーネントのことまで知っているというのはおかしい（保守性が落ちる）ので、↓みたいに書けるようにしたいです。（方法わかります？）

import type { HandleSubmitEditedTodo, HandleToggleCompleted, HandleClickRemove } from "@/components/TodoList.vue";

Copy link
Owner Author

Choose a reason for hiding this comment

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

下のように修正しました。

// TodoList.vue

export type { HandleToggleCompleted, HandleClickRemove }

@konti-kun
Copy link

konti-kun commented Jan 4, 2021 •
edited
Loading

あとeslintのルールは、少なくても慣れるまではplugin:vue/vue3-essentialではなくてplugin:vue/vue3-recommendedとするほうがオススメです

これがいい感じの方法が見つからないんですよね。

    '@typescript-eslint/naming-convention': [
      'warn',
      {
        selector: 'objectLiteralProperty',
        format: ['PascalCase', 'camelCase'],
      },
    ],

基本はcamelCaseだけどcomponentsの時だけPacalCaseを許すというルール。オプション使えばいけるのかな。

@aki77
Copy link

aki77 commented Jan 4, 2021

基本はcamelCaseだけどcomponentsの時だけPacalCaseを許すというルール。オプション使えばいけるのかな。

PacalCaseが不適切なケースでも、ついミスってPacalCase使ってしまうということが無いのであれば設定例のように両方許容しておけば良さそう！？

@aki77
Copy link

aki77 commented Jan 9, 2021

良さそう！
お疲れ様でしたー。 👍

@r-toki
Copy link
Owner Author

r-toki commented Jan 10, 2021

たくさんのレビューありがとうございました！
すごく勉強になりました。

@r-toki r-toki merged commit b1d022b into main Jan 10, 2021
@r-toki r-toki mentioned this pull request Jan 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aki77 aki77 aki77 left review comments

@konti-kun konti-kun konti-kun left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@r-toki @aki77 @konti-kun