-
Notifications
You must be signed in to change notification settings - Fork 305
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
Add: トラック毎の書き出しを追加 #2228
Add: トラック毎の書き出しを追加 #2228
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.
draft PRありです!!
お待ちしています・・・!
@@ -2252,6 +2258,148 @@ export const singingStore = createPartialStore<SingingStoreTypes>({ | |||
), | |||
}, | |||
|
|||
// TODO: EXPORT_WAVE_FILEとコードが重複しているので、共通化する |
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.
共通化やっぱり難しそうですかね・・・?
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.
ん~どうでしょう、でもあんまり共通化できそうなところもないので消しても良さそう...?
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.
たぶん音声出力は3箇所目なのとかなり長いのとで、流石に共通化できると嬉しそう。
いずれ書き出し設定したあとに書き出すダイアログを作るときに共通化しないといけなくなるはずだし。
まあでもぎりぎりこのプルリクのタイミングじゃなくても良さそう。
してあげといた方が後続の助けになりそうだな〜って感じかなと!
(このままだとマルチトラック書き出しがどんどん複雑になって、新しい実装をする前に交通整理が必要になる。マルチエンジンみたいに。)
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.
一通りできました。
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.
提案2つあるので2つコメントしました!
(スレッド分けたかったので適当なファイルにコメントしました)
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.
トラックごとの書き出し機能、人によっては何度も何度も実行すると思うので、ショートカットキー実装しちゃうのどうでしょう?
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.
ソングとトークで同じショートカットキーを設定出来ないっぽいので、それの修正と一緒に別PRでやろうとおもいます。
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.
うおーたすかります!!
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.
何度も修正&書き出しする場合、たぶん「書き出し先を固定する」とかなり相性が良さそうなのですが、どうでしょう・・・?
ソング側でショートカットキーで書き出し
→ DAW側で勝手に再読込される(されるんだろうか)
→ 聞く
→ ソング側で微調整
→ 以下繰り返し
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.
実際あったら嬉しいですね。
#1250
これと一緒に実装…?
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.
組み合わせ良さそうですね!!
そのissueはUI/UXが自明じゃないので、少なくともこのPR内でのついでに実装はレビューが厳しくなる予感があります。
とりあえずそちらのissueについては、まずはissue内でUI/UXの検討を進めるとかどうでしょ?
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.
ですね。
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.
細かいとこ含めてコメントしてみました!
export waveを共通化すべきかとか辺りまだ見れてないです🙇
あっすみません! こちら再レビューしても大丈夫そうでしょうか 👀 |
あ~Request Review忘れてました |
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.
ちょっと細かいとこが多いですが、プログラミング周りのことをコメントしました!!
あんま興味なければ言っていただければ🙇
@@ -2252,6 +2258,148 @@ export const singingStore = createPartialStore<SingingStoreTypes>({ | |||
), | |||
}, | |||
|
|||
// TODO: EXPORT_WAVE_FILEとコードが重複しているので、共通化する |
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.
たぶん音声出力は3箇所目なのとかなり長いのとで、流石に共通化できると嬉しそう。
いずれ書き出し設定したあとに書き出すダイアログを作るときに共通化しないといけなくなるはずだし。
まあでもぎりぎりこのプルリクのタイミングじゃなくても良さそう。
してあげといた方が後続の助けになりそうだな〜って感じかなと!
(このままだとマルチトラック書き出しがどんどん複雑になって、新しい実装をする前に交通整理が必要になる。マルチエンジンみたいに。)
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.
storybookのテスト落ちてるの何でなんだろ。。。。
src/components/Dialog/SettingDialog/FileNameTemplateDialog.stories.ts
Outdated
Show resolved
Hide resolved
src/components/Dialog/SettingDialog/FileNameTemplateDialog.stories.ts
Outdated
Show resolved
Hide resolved
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です!!
パッチもありがとうございます!!
src/components/Dialog/SettingDialog/FileNameTemplateDialog.stories.ts
Outdated
Show resolved
Hide resolved
src/components/Dialog/SettingDialog/FileNameTemplateDialog.stories.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Hiroshiba <[email protected]>
Co-authored-by: Hiroshiba <[email protected]>
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.
結局postinstallが必要で申し訳ない😇
ちょっとrootにディレクトリ増やしたくない思いがあり、patchesディレクトリを移せないかと思ってます。
buildディレクトリ以下に置いても良いでしょうか?
(ドキュメント読んだ感じ --patch-dir で変更できそう)
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!!!
長らくありがとうございました!!!
良い機能だと思います!!
内容
stem書き出し機能を追加します。
関連 Issue
スクリーンショット・動画など
その他
(なし)