-
Notifications
You must be signed in to change notification settings - Fork 309
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
マルチトラック:インポートに対応 #2126
マルチトラック:インポートに対応 #2126
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です!!
もしよければ、マルチトラック用の何かしらのimport&objectスナップショットテストあると安心かもです。
if (project.value == null || selectedTrack.value == null) { | ||
if (project.value == null || selectedTrackIndexes.value == null) { |
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.
他と合わせて、空っぽのときもエラーでも良いかも
(そのままでも良いかも)
このあたりの処理(disableになってるかとか)をstorybook辺りでテスト書けるとかっこいいかもですね
src/store/singing.ts
Outdated
const isTracksEmpty = (tracks: Map<TrackId, Track>) => | ||
tracks.size === 1 && [...tracks.values()][0].notes.length === 0; |
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.
こういうのがいっぱい詰め込まれたutitliyあった記憶。
そちらに似た関数あればそちらに書くほうがまとまり良いかも。
あとsize==0のときもempty判定で良いかも?
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.
sing/domain.tsにisValid系がたくさんありましたが、Map<TrackId, Tracks>を使ってるのはここくらいしかないのでここに置くのがいいかな~って感じでした。
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.
domain.tsに置くとsinging.tsがすっきりするので、domain.tsに置いても良いと思います。
また、この関数ではkey(TrackId)は必要ないので、引数の型はTrack[]
でも良いかもです。
isTrackEmpty: (track: Track) => boolean
にして
tracksArray.length === 1 && isTrackEmpty(tracksArray[0])
でも良いかも。
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です!
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!!
ちょっとこちらでUIの調整をさせていただこうと思います!
(詳細開くとはみ出る点は未解決ですが。。)
startFrame: 0, | ||
trackId, | ||
}); | ||
// 色々な処理を動かすため、二重にセットする |
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.
ここも忘れないようにTODOコメント入れさせていただきます 🙇
内容
インポートをマルチトラックに対応させます。
関連 Issue
スクリーンショット・動画など
その他
(なし)