-
Notifications
You must be signed in to change notification settings - Fork 298
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
ソング: 正式なカラーとスタイルを定義する #2218
base: main
Are you sure you want to change the base?
ソング: 正式なカラーとスタイルを定義する #2218
Conversation
…com/romot-co/voicevox into feature/1810_sing_colors_and_styles
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.
実装周りの詳細な点を除きつつ、一旦全体的な点に関してコメントしました!
デザインすごくいいと思います!!!
ピアノロールの部分かなり違和感なくて驚きました!!
デザインに関して何点かだけちょっとだけ気になったとこがあったのでコメントさせていただきます!
(変えて欲しいというより、意図があればお聞きしたいみたいな感じです!)
ノート端ホバー時の太くなり方がちょっと大げさかも?
(これはどっちかというとノート内だけに線が太くなってるから違和感があるだけかも。だとしたら実装的に変更が難しそうなので、今の感じでも・・・!)
今後Quasarが必須でなくなるのであればradix-vueに変更したい
必須ではなくなっていく予定です・・・!!
src/styles/v2/variables.scss
Outdated
--font-regular: 400; | ||
--font-medium: 500; | ||
--font-bold: 700; | ||
|
||
--input-m-size: calc(var(--size-basis) * 5); | ||
--input-l-size: calc(var(--size-basis) * 7); | ||
|
||
--label-l-size: calc(var(--size-basis) * 1.75); | ||
--label-m-size: calc(var(--size-basis) * 1.5); | ||
--label-s-size: calc(var(--size-basis) * 1.25); | ||
|
||
--border-width-basis: 1px; | ||
|
||
--opacity-medium: 0.87; | ||
--opacity-low: 0.6; | ||
--opacity-separator: 0.5; | ||
--opacity-disabled: 0.38; |
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.
@Hiroshiba
こちら消し忘れです!
いちおうtakuseaさんのv2とあわせようと思って修正しようとしたのですが
(追加したものというよりは、すでにあるgapやpaddingやradiusを使っていたもののそちらは削除済み
たぶんどう使うか・変更するか擦り合わせてからのがいいかなーと思っております!
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.
おお、なるほどです!
たしかに合わせていけると良さそう。
src/styles/v2/sing-colors.scss
Outdated
/* | ||
フォールバック | ||
*/ | ||
|
||
@supports not (color: oklch(0% 0 0)) { | ||
:root[is-dark-theme="false"] { | ||
--scheme-color-primary: #29683a; |
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.
ここのフォールバックなるほどです!!
こちらに関して、メンテナンス面考えるとどういう風に扱っていくのがいいでしょう? 👀
上に値を足したり値を変更したら、こっちも更新する感じとか?
(結構大変そう&忘れそうだな~~~~と。。)
候補考えてみました!
- なるべく書くということにして、忘れてもまあ別にいいことにする
- 書き忘れていないかチェックするテストを書く
- 自動更新するスクリプトを用意する
- scssを頑張って自動でoklabとrgbどちらも出力するようにする
- 思い切って無しにする
個人的には2+3か、あるいは5も結構ありなのかなと思ってます!
5にしておいて、将来未サポートなブラウザもサポートする際にフォールバックと2+3を用意するとか・・・!
(変換スクリプトをすでにお持ちでしたら、そのスクリプトを掲載する・あるいはbuild
ディレクトリに放り込むとかも現実的かも?)
といっても意外とoklchは全ブラウザの最新版ではれサポートされてそう?
oklabの相対色指定は現状safariでサポートされてないものもありそう。
https://developer.mozilla.org/ja/docs/Web/CSS/color_value/oklch#%E3%83%96%E3%83%A9%E3%82%A6%E3%82%B6%E3%83%BC%E3%81%AE%E4%BA%92%E6%8F%9B%E6%80%A7
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.
@Hiroshiba
こちら元々がOKLCHおよびフォールバックの両方をスクリプトで出力しているものなため、
3.がいいのかな…?と思っております。
4.ができるかもあわせて検討試行してみます!
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.
SASSでOKLCH→その他の形式に変換はむずかしそう
今後、動的テーマ・カスタムテーマに対応するなら同時にHEX出力すれば大丈夫だが
それまでは3.をがんばるか5.にするか
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.
たしかに動的テーマに対応する場合、必ずjs入るからjsで解決できますね!!
こちら元々がOKLCHおよびフォールバックの両方をスクリプトで出力しているものなため、
3.がいいのかな…?と思っております。
おお、なるほどです!!
そちらがあるならたしかに3も近そうですね!
ただ、あとあと動的テーマのときに解決されるのであればパスしても(=5でも)良いかも・・・・・・?
判断むずいですね!!
個人的には特に他の理由がなければ5寄りの意見です!
(変更用コードはNodeJSで動くようにしないとで結構大変な気がしたので)
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.
@Hiroshiba
ありがとうございます!
簡単な手段も見当たらないため、こちらいったん5にします…!
@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.
d76bff1
to
0af8e2f
Compare
…com/romot-co/voicevox into feature/1810_sing_colors_and_styles
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です!!!
ちょっと色々だいぶ細かいコメントをさせていただきました!!
(今コメントでお伝えすれば、きっと今後ずっとお互いやりやすくなるだろうと思い・・・!)
色定義のSCSSファイルで詳細なドキュメントを書いてくださってありがとうございます!!
どうにかしてオープンソースとしてメンテナンスしやすい色定義の形を模索したいですね・・・!!!
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
こちらSASSで関数利用するための 修正いたしました! (Quasarもdeprecatedが出ているのですが両方バージョンアップは影響範囲広そうなのいったんこのままで… |
@Hiroshiba
こちらありがとうございます…!
メンテしづらい部分があるので またScoreSequencerおよびSequencerNoteの方
今よりは修正しやすくしたつもりではあるのですが |
'rect-selecting': editTarget === 'NOTE' && shiftKey, | ||
previewing: nowPreviewing, | ||
'add-note': previewMode === 'ADD_NOTE', | ||
'resize-note-right': previewMode === 'RESIZE_NOTE_RIGHT', | ||
'resize-note-left': previewMode === 'RESIZE_NOTE_LEFT', | ||
'move-note': previewMode === 'MOVE_NOTE', | ||
'draw-pitch': | ||
(editTarget === 'PITCH' && !ctrlKey) || previewMode === 'DRAW_PITCH', | ||
'erase-pitch': | ||
(editTarget === 'PITCH' && ctrlKey) || previewMode === 'ERASE_PITCH', |
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.
おーーーーなるほどです!!!
ScoreSequencer.vue
側にマウスカーソルの形変更を入れるんですね!!
確かに思いつかなかった、こっちのがいいですね!!
これによってSequencerNote側にある、条件ごとのカーソル変更が不要になったかも・・・・・?
例えば↓とかの.moving
クラス内のcursor: move;
とか。
https://github.com/romot-co/voicevox/blob/8e04c7db683b4b96b81ac7d1641d0854268a53cc/src/components/Sing/SequencerNote.vue#L383-L390
でもそのままでもいいといえば、そのままでも良いかもですねぇ。
カーソルだけはScoreSequencer.vue
のみにする・・・・・?
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.
こちらScoreSequencer側修正しました
src/composables/usePreviewMode.ts
Outdated
const previewMode = ref<PreviewMode>("IDLE"); | ||
const nowPreviewing = computed(() => previewMode.value !== "IDLE"); | ||
const executePreviewProcess = ref(false); |
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.
!!!!!
Vueってグローバル変数にrefとかcomputedとか定義しても動くんですね!!!!!!
でもコンポーザブルでこれをやって大丈夫なのかわからないので(ドキュメントには書いてなさそう、実装レベル知ってると分かりそう)、ちょっと避けとくのどうでしょう・・・・・・・・?
もし避けとく場合は、SequencerNote側でusePreviewMode
を使うのをやめて、previewMode
とnowPreviewing
をpropsで渡すように戻すと良さそう・・・!!
ちなみに今回は親子構造だから割と簡単にこうやって引数渡せますが、もっと深いところとか兄弟に渡そうとすると不便なんですよね。
そういう時はProvide / Inject
を使ったりするのが若干一般的かもです。
https://ja.vuejs.org/guide/components/provide-inject#prop-drilling
(Vuexはこの仕組みを使ってどこからでも使えるようになってます。・・・・・たしか。)
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.
ありがとうございます!
グローバル変数は避ける&もしやるなら現状だとVuexで管理だと思うので
usePreviewMode削除して(現状だと再利用できなさそう&そのまま残しておくと危なそう)
ScoreSequencerからSequencerNoteに渡す形にいたしました
Provide/Injectの方ありがとうございます!
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です!!!!!!!
長くありがとうございました!!!!!
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
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.
left: -0.5px; | ||
width: 1px; | ||
background: var(--scheme-color-inverse-primary); |
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.
のちのちですが: Windows環境で調査
こちらマージ後、別の形で修正できればと思います! |
内容
ソング機能において正式なカラーを定義し、スタイルを調整することでユーザーの使い勝手を向上させます。
一定程度のアクセシビリティ対応を含みます。
また、今後の拡張を容易にします。
色関連の修正
※ 動的なテーマ生成+テーマエディタ機能のサブセットとなります
表示上の調整
Toolbar
※ 今後Quasarが必須でなくなるのであればradix-vueに変更したい
ScoreSequencer/SequencerGrid/SequencerKeys/SequencerRuler:
SequencerNote
LyricInput
SequencerPitch
pros: ガイド線となり無いよりは描きやすい・基準ピッチからのずれを把握しやすい
cons: ガイド線に引っ張られた描き方となる
関連 Issue
ref #1810
close #1810
スクリーンショット・動画など
ノート編集
ピッチ編集
その他
スタイルにおいての変数の使用や書き方については、別途Issue化+すりあわせができればと思います。
設計についての考え方