Skip to content

Conversation

@romot-co
Copy link
Collaborator

@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.

遅れてすみません、レビューしてみました!!
特に大きく変更が必要そうな箇所は見当たりませんでした!
いくつかコメントしてみたので、気に入ったのがあれば改修&再レビュー&マージかなと!

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とかでも良いかもと思いました。

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がイベントを受け取って操作を制御する設計、思いつきませんでした!
なるほどです、良さそう。

Copy link
Collaborator

@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
Collaborator

@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.

(すみませんコメントが送信できていませんでした…!)

@romot-co
Copy link
Collaborator Author

romot-co commented Jul 1, 2023

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

  • Noteにuuid付与

が主な変更点です。

Copy link
Collaborator

@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箇所だけコメントしていますので、おてすきでご確認お願いいたします!

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 Hiroshiba merged commit 554afef into project-s Jul 10, 2023
@romot-co
Copy link
Collaborator 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.

4 participants