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: v-for1つでノートを描画する形に変更し、ダブルクリック判定処理を無くす #2118

Merged
merged 16 commits into from
Jun 23, 2024

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Jun 9, 2024

内容

v-for1つでノートを描画する形に変更し、ダブルクリック判定処理を無くします。

また、以下も行います。

  • onNoteLyricMouseDownを削除する
  • ScoreSequencerのcontextMenuDataがcomputedになっていなかったので修正
  • 編集のプレビュー中はノートの右クリックメニューをdisableにする
  • 歌詞入力をコンポーネント化する

関連 Issue

ref #2041

その他

ノート編集時の動作が若干重くなってるかもです…

@sigprogramming sigprogramming requested a review from a team as a code owner June 9, 2024 12:49
@sigprogramming sigprogramming requested review from Hiroshiba and removed request for a team June 9, 2024 12:49
contextMenu.value?.hide();
await store.dispatch("COPY_NOTES_TO_CLIPBOARD");
},
disabled: !isNoteSelected.value,
Copy link
Contributor Author

@sigprogramming sigprogramming Jun 9, 2024

Choose a reason for hiding this comment

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

isNoteSelectedがリアクティブなので、contextMenuDataをcomputedにしました。

@sigprogramming sigprogramming changed the title v-for1つでノートを描画する形に変更し、ダブルクリック判定処理を無くす Refactor: v-for1つでノートを描画する形に変更し、ダブルクリック判定処理を無くす Jun 9, 2024
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/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
@sigprogramming sigprogramming requested a review from Hiroshiba June 15, 2024 14:21
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.

+350 −393はちょっと変更量が多いですね・・・!
歌詞コンポーネント切り出しは別PRだと助かったかもです。

ちょっと気合い入れて眺めてみます!!

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

いくつかコメントしていますが、マストで変更したほうが良さそうだと感じたとこは特に無いです!

変更量多いかもと思いましたが、意外と見れました。
HTML部分とstyle部分が含まれると変更行数に比べて比較的楽そう。
とはいえここからさらに変更だと厳しいかもです!すみません!! 🙇
(並行していくつもPR見ていると、前のレビューの記憶が抜けてしまっており。。。)

src/components/Sing/SequencerLyricInput.vue Show resolved Hide resolved
src/components/Sing/SequencerLyricInput.vue Show resolved Hide resolved
src/components/Sing/SequencerLyricInput.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
Comment on lines +59 to +61
<SequencerLyricInput
v-if="editingLyricNote != undefined"
:editingLyricNote
Copy link
Member

Choose a reason for hiding this comment

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

(コンポーネント設計うんちくです)

表示の切り替え方法について、v-ifにするパターンと、props.editingLyricNoteをundefinedableにしてSequencerLyricInput内でコンポーネント内で表示非表示を切り替えるパターンがあり、どちらが良いか迷ってました。
今回はv-ifでも良さそうでした!

というのも、v-ifを使うとtrue/falseが切り替わったタイミングでコンポーネント内のstateが消えるんですよね。
v-ifの場合、もし歌詞入力中に一度SequencerLyricInputを非表示にすると、入力中の歌詞が消えるなぁと。
今回の場合は歌詞入力が消えてほしくないタイミングでv-ifで消すことがないので、どちらでも大丈夫そう!

@sigprogramming
Copy link
Contributor Author

レビューありがとうございます!
変更量が多くなってしまいすみません…!

@sigprogramming sigprogramming requested a review from Hiroshiba June 22, 2024 22:50
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/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit a4d0fa9 into VOICEVOX:main Jun 23, 2024
8 checks passed
@sigprogramming sigprogramming deleted the refactor_sequencer_note branch June 23, 2024 10:36
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