-
Notifications
You must be signed in to change notification settings - Fork 311
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: SequencerPitchをリファクタ #2510
refactor: SequencerPitchをリファクタ #2510
Conversation
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
1点だけこちらで変更させていただこうと思います! 🙇
ちょっとずうずうしいお願いなのですが、もしできれば今度からコードの移動とコードの変更はコミットを分けていただけるとありがたいです・・・!
移動と変更を同時にすると、PR内でbefore/afterでコードの変更箇所がわからなくなってしまい。。(gitが差分として見つけられない)
移動と変更が別PRだと最高ですが、移動だけ最初にコミットとかでもかなり助かります 🙏
(ちなみにこれはGoogleのベストプラクティス似たことが書いてたりします。)
https://google-engineering-practices.translation.shuuji3.xyz/review/developer/small-cls.html
@Hiroshiba |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
いろいろ注文付けてしまってすみません。。 🙇
リファクタリングはウェルカムです、ぜひぜひ!!!
ちょこっと思ったのですが、たぶん今回のリファクタリングは将来描画を自前で行う(今は一部VueやHTMLを使ってる)のも見据えての感じなのかなと思います。
HTMLをあまり使わないのは賛成よりですが、レンダリングに関してはVueのreactive(refとかcomputedとかwatchとか)ドリブンでも良いのかもと少し思いました!
毎フレーム描画だとわりと重い処理だと思うのと、Vueのreactiveの機構が特に計算負荷にならなそうだなーと思い。
・・・ここまで書いて気づいたのですが、このプルリクエストはまさに「値が変わったらレンダリングする」になってますね!!!!!
内容
SequencerPitchのリファクタリングです。
主に以下を行います。
{ ticksArray: number[]; data: number[] }
から{ baseX: number; baseY: number }[]
に変更toPitchData
、splitPitchData
、setPitchDataToPitchLine
の処理をtoPitchDataMap
にまとめるその他