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

[project-s] ノート(複数)をドラッグおよびキーボードで移動+長さ変更+削除 #1256

Merged
merged 47 commits into from
Jul 10, 2023

Conversation

romot-co
Copy link
Contributor

@romot-co romot-co commented Mar 19, 2023

内容

Noteについて、以下の各操作を行えるようにします。

  • 複数ノートの選択/解除
  • 選択中のノートをドラッグで移動
  • ノート左右ドラッグで長さの変更
  • 選択中のノートをキーボードキーで移動

※ 試行用ブランチではなく別ブランチとし、リファクタリングしてからマージできればと思いましたが、
差分が大きくなりそうなためいったんマージできれば幸いです。

関連 Issue

ref #1114
close #1114

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

test-vvs.mov

その他

現時点で、以下の問題点があります。

  • ノート追加がダブルクリックになっている
  • ドラッグなどの操作時にscore.notesをそのまま変更している(移動などが終わってからscore.notesにセットした方がよさそう)
  • キーボード操作が既存の方法に沿っていない
  • メソッド共通化や可読性の向上などリファクタリングが行われていない
  • Vuexのstore側で処理した方がよさそうなものがあるほか、コンポーネントが密結合

Romot and others added 30 commits November 4, 2022 22:20
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/helpers/singHelper.ts Outdated Show resolved Hide resolved
Comment on lines +139 to +140
const cursorX = ref(0);
const cursorY = ref(0);
Copy link
Member

Choose a reason for hiding this comment

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

今は常に更新されていますが、必要なときだけ取ってこれるような仕組みにできるとかっこいいかもと思いました!
細かいですが、ドラッグ中用のだとわかるようにdragCursorXとかでも良いかもと思いました。

src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Show resolved Hide resolved

const setLyric = (event: InputEvent) => {
if (!(event.target instanceof HTMLInputElement)) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

細かくてすみませんがもしよければthrowだと・・・!
(仮になにかのミスでこの関数が呼ばれたとき、なにもせずreturnになってしまってエラーがわからなくので)

Suggested change
return;
throw new Error("event.target is not HTMLInputElement");

ちなみにGithub Copilot使っているとthrowと書くだけでそれっぽいエラーメッセージが出てくるので便利だったりします!

Comment on lines +118 to +120
const handleKeydown = (event: KeyboardEvent) => {
emit("handleNotesKeydown", event);
};
Copy link
Member

Choose a reason for hiding this comment

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

activeになっているDOM Componentがイベントを受け取って操作を制御する設計、思いつきませんでした!
なるほどです、良さそう。

src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sigprogramming sigprogramming left a comment

Choose a reason for hiding this comment

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

レビュー遅くなり失礼しました…!
いくつかコメントしていますが、ほぼLGTMです!

Copy link
Contributor

@sigprogramming sigprogramming 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/singing.ts Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
@romot-co
Copy link
Contributor Author

romot-co commented Jul 1, 2023

@Hiroshiba @sigprogramming
(おてすきで)遅くなり失礼いたします、こちらいったんマージできる形に修正いたしました。
おてすきでご確認お願いいたします!

  • Noteにuuid付与

が主な変更点です。

Copy link
Contributor

@sigprogramming sigprogramming left a comment

Choose a reason for hiding this comment

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

コンフリクト解消&修正ありがとうございます!
1箇所だけコメントしていますので、おてすきでご確認お願いいたします!

src/store/singing.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!!!!!

(余談です)
マルチセレクト、すごいです。
エディタ側は最初から複数選択できるように作っていなかったので、今から置き換えるのに相当苦労するなという見積もりになっています。
最初から設計に加えておけばよかったなと思いました 😇

src/store/singing.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 554afef into project-s Jul 10, 2023
@romot-co
Copy link
Contributor Author

@Hiroshiba
お忙しいところありがとうございます!

複数選択(複数ノートへの一括操作)はわりと使う気が勝手にしていたのですが、思い込みかもしれず
まずはなくてもいいかもです…!

@Hiroshiba Hiroshiba deleted the feature/1114_note_select_and_change_duration branch January 28, 2024 17:24
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