Change: ショートカットキー周りをHotkeyManagerにまとめる #1822
Conversation
| @@ -110,7 +110,6 @@ | |||
| "license-checker": "25.0.1", | |||
| "markdownlint": "0.31.1", | |||
| "markdownlint-cli": "0.37.0", | |||
| "mousetrap": "1.6.5", | |||
There was a problem hiding this comment.
|
コメントとかを足しました。 |
|
onMounted/onUnmountedはテストだと動かないじゃん... |
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
|
良い感じになったと思います。 |
There was a problem hiding this comment.
なるほど！！！ nameとeditorをまとまりにする感じですね！！ 良さそうに思いました！！
ちょっと各所にちらばる .name == .name && .editor == .editor をまとめといた方が良さそうに思いました。
nameとeditorをまとめてActionTargetとかにし、isSameTarget(a, b)関数を用意して、filter内でその関数を使うのはどうでしょう。
あるいはtargetを指定してgetする関数とdeleteする関数を作っちゃうのもありかも。
こんな感じ。
getWithTarget<T extends ActionTarget>(objs: T[], target: ActionTarget): T
omitWithTarget<T extends ActionTarget>(objs: T[], target: ActionTarget): T[]
|
高階関数として実装してみました。 |
There was a problem hiding this comment.
LGTMです！！
こちらで1回リファクタリングしてプッシュしてみます！
ので @sevenc-nanashi 側でレビューいただいて、もらえなければマージさせていただければ！
| /** エディタの種類 */ | ||
| editor: "talk" | "song"; |
There was a problem hiding this comment.
この引数を受け取る設計、本当は良くないのですが、今の状態からだとベストな気がしました！
MenuBarを１つにしたほうが良いと思うので、ちょっとそれはそれでissueを作ってみました！
| describe("設定変更", () => { | ||
| let hotkeyManager: HotkeyManager; | ||
| let dummyHotkeysJs: DummyHotkeysJs; | ||
| beforeEach(() => { |
There was a problem hiding this comment.
これはちょっと自信がないので、コメントだけ･･･！！
beforeEachで作ったデータを各々のテストで使う形にすると、大元のデータを変えたくなった時に結構しんどくなっちゃうんですよね。
あと前提条件（データ作成部分）とテスト箇所が離れちゃうので、間違ったテストを書いていても気づきにくいというのもあります。
どちらかというとデータを作る便利関数を作ってあげて、必要最低限のテストを書くのが良い気がしました！
今回だとactionやcombinationは自動生成できるので。
それはそれとして実利用想定の大きめのテストあった方がいいと思うので、登録したり消したりなんか変な操作したりといった一連の操作を行うテストもあってもいいかも。
|
ほんのり弄りました。 |
| const callback = () => { | ||
| /* noop */ | ||
| }; | ||
| const dummyAction: HotkeyAction = { | ||
| editor: "talk", | ||
| name: "音声書き出し", | ||
| callback, | ||
| }; | ||
| const createDummySetting = (combination: string): HotkeySettingType => ({ | ||
| action: "音声書き出し", | ||
| combination, | ||
| }); |
There was a problem hiding this comment.
（変えるほどのことでもないのでコメントだけ）
影響範囲と同じスコープ（describe("設定変更", () => {の中）で定義する形もありかもですね！
|
マージ後にすみません。。。macで大半のショートカットキーが使えなくなったことに気づきました 🙇 おそらく |
内容
ショートカットキー周りをまとめたHotkeyManagerを作りました。
このPRはソングエディタのショートカットキー構築の一環です。
関連 Issue
スクリーンショット・動画など
（なし）
その他
（なし）