Skip to content
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: BPM・拍子変更機能を追加 #2303

Open
wants to merge 105 commits into
base: main
Choose a base branch
from

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Oct 18, 2024

内容

タイトル通りです。

関連 Issue

close #1919

スクリーンショット・動画など

image

その他

現時点でもダイアログの挙動自体はStorybookから確認出来ます。

() => lastTimeSignature.value.measureNumber === currentMeasure.value,
);

const contextMenuHeader = computed(() => `${currentMeasure.value}小節目`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは #2306 の形式で出そうかな~って思ってます。

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review October 19, 2024 16:16
@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner October 19, 2024 16:16
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team October 19, 2024 16:16
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.

📝 const hoveredTempoOrTimeSignatureChange = ref<number | null>(null);辺りまで見ました!

主に UI 回りや、ちょっとした実装周りについてコメントしています。

ちょっと色んな事情を知らずにコメントしてる点もあると思うので、違和感あったりしたら結構気軽に教えていただけると 🙇

src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
Comment on lines 101 to 106
@click.stop="
onTempoOrTimeSignatureChangeClick(
$event,
tempoOrTimeSignatureChange,
)
"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっとUI迷ってるのですが、左クリックと右クリックが同じ動作なの不思議かもとちょっと思いました。
可能なら、左クリックは「編集」開始が良いかも・・・?
ただその場合だと、一括編集UIを用意するか、個別にクリック可能にする必要が出てきそう。。。。

うーん! 一旦コメントまで🙇
(意見聞きたみ!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

例えば圧縮されているときにどうするのとかそう言うのがあるので編集開始は考えることが多そう?
ダイアログ自体分かれてるのでBPM/拍子をクリックしても一段階メニューが挟まるのは自然だと思います。

src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
Comment on lines +45 to +54
const screenshots = await fs.readdir(
`${import.meta.dirname}/スクリーンショット.spec.mts-snapshots/`,
);
for (const screenshot of screenshots) {
if (!currentStories.some((story) => screenshot.startsWith(story.id))) {
await fs.unlink(
`${import.meta.dirname}/スクリーンショット.spec.mts-snapshots/${screenshot}`,
);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ただのコメントです)

不要なの消えるようにしてくださった感じですよね、ありがとうございます!!! 便利!!

もしよければこんな感じでプルリクと完全に分けられるものはサクッと別プルリクエストいただけると結構嬉しかったりします!
まあ割とでかいものにくっついてるとなかなかマージされなかったりするので、くらいの違いですが・・・!

とはいえmainブランチと行き来するのちょっと面倒だとは思うので、こんな感じで何かにくっつけてでももちろんかなりありがたいです 🙏

src/components/Sing/SequencerGrid/Presentation.vue Outdated Show resolved Hide resolved
@romot-co
Copy link
Contributor

@Hiroshiba

@romot-co さんのちょっと変更意見なるほどです! これってつまりテンポ・拍子が一緒になっているときは一緒に編集・削除を可能にして、新規挿入する場合も一緒に突っ込まれる・・・みたいな感じでしょうか 👀

ちょっと素人的意見なので微妙なこと言ってるかもなのですが、テンポと拍子って大体の場合片方だけいじりたいことの方が多いのかな~と思っていて、だとしたら興味あるものだけを表示して操作可能にした方がわかりやすいかも・・・・?と個人的に思いました!

一緒につっこむべきというよりは、現状は実質両方編集できる形なのでそれを素直に表したほうがいいかも?ぐらいの意図となります!
削除の件が大きいです


おっしゃるとおりいじりたいものだけいじれればいい形かと思います!

個人的には拍子・テンポ・あとキーなどは別個に扱って、
それぞれレーン自体をわけるのがいいかなーと思っています…!

各要素追加できる・編集できる・削除できる・移動できる…などを考えると
追加したらマーカー「3/4」などが追加され、それを選択して右クリックやダブルクリックで編集・削除、必要あれば移動…といった感じかなーと

どちらかというと、削除わかりづらいかも?が大きかったので、

メニューでの表示を「拍子を削除」「テンポを削除」「削除」にするとかもありかも。(さすがに並びすぎて微妙かも・・・)

このメニューに削除追加する形でよさそうです…!

@sigprogramming
Copy link
Contributor

@romot-co
もしかしたらローカルブランチが最新でないかもです…!
(ピッチ表示が直っているのと、テンポと拍子でダイアログが分かれてます)

@romot-co
Copy link
Contributor

@sigprogramming x
ありがとうございます!
こちら最新の取り込み漏れでした(ごめんなさい…!

src/helpers/textWidth.ts Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerRuler/Presentation.vue Outdated Show resolved Hide resolved
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