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

複数選択:キャラクターを変更できるように #1546

Merged
merged 9 commits into from
Sep 19, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Sep 12, 2023

内容

タイトル通り。

関連 Issue

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

VOICEVOX.-.Ver.999.999.999.Mozilla.Firefox.2023-09-18.11-32-25.mp4

その他

(なし)

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review September 12, 2023 13:02
@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner September 12, 2023 13:02
@sevenc-nanashi sevenc-nanashi requested review from y-chan and removed request for a team September 12, 2023 13:02
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.

アサイン僕じゃなかったんですけどコード読みやすくてついレビューしてしまいました!
すごい変更を読みやすかったです。

src/store/audio.ts Outdated Show resolved Hide resolved
tests/e2e/browser/複数選択/common.ts Outdated Show resolved Hide resolved
tests/e2e/browser/複数選択/操作.spec.ts Outdated Show resolved Hide resolved
tests/e2e/browser/複数選択/操作.spec.ts Outdated Show resolved Hide resolved
const isInitializingSpeaker = computed(
() => store.state.audioKeyInitializingSpeaker === props.audioKey
const isInitializingSpeaker = computed(() =>
store.state.audioKeysInitializingSpeaker.includes(props.audioKey)
Copy link
Member

Choose a reason for hiding this comment

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

これ2回出てきてるので、AudioKeyを引数にしたGETTER関数isInitializingSpeakerにしてあげてもいいかも。
まあシンプルなのでそのままでもいいかも。

Copy link
Member Author

Choose a reason for hiding this comment

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

シンプルなのでそのままで良いと思います

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

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かなという気持ちです!!

src/store/audio.ts Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

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.

実行してみたらエラーがconsole出力されていました。
npm run typecheck:vue-tscすると変更漏れ箇所がわかるかもです

@sevenc-nanashi
Copy link
Member Author

修正しました。

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!!!!!!!!

いやーーーーーーー実際使ってみるとすごい感動するくらい便利ですね!!!!!!
他の機能もぜひ実装したみ!!!!!!!

@Hiroshiba
Copy link
Member

問題なさそうなのでマージします!

@Hiroshiba Hiroshiba merged commit 6c683c8 into VOICEVOX:main Sep 19, 2023
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.

2 participants