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

Refactor: SequencerRulerをContainer/Presentationに分離 #2312

Conversation

sevenc-nanashi
Copy link
Member

内容

タイトル通りです。

関連 Issue

スクリーンショット・動画など

(なし)

その他

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner October 22, 2024 20:54
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team October 22, 2024 20:54
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!!

1つミスっぽかったのでコメントしました!

あとちょっと変更できそうな点をいくつかコメントにしてみました。
すでにこちらで試したので、ついでにプルリクエストも作ってみました。
その産物がこちらのPRです。

src/components/Sing/SequencerRuler/Container.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/index.stories.ts Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/index.stories.ts Outdated Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

分離ありがとうございます!!
一部だけちょっと細かいとこ変えさせていただきます!

Comment on lines 40 to 43
args: {
"onUpdate:playheadTicks": fn<(value: number) => void>(),
onDeselectAllNotes: fn(),
},
Copy link
Member

Choose a reason for hiding this comment

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

基本全部meta.argsに突っ込むのがベストかな〜〜〜と思ってます!
追加で必要になったときにコピペしないといけないので、デフォルト挙動としてmeta.argsに置いとくのが良さそうかなと。

まあそれが正解かわからんですが、一旦他のstoriesはそうなってるので合わさせていただきます!
(個別のが良いなってなったら全部そうしちゃいましょう!!)

@Hiroshiba
Copy link
Member

マージします!

@Hiroshiba Hiroshiba merged commit d7087ef into VOICEVOX:main Oct 23, 2024
8 checks passed
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