-
Notifications
You must be signed in to change notification settings - Fork 308
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
Update: キー割り当て設定画面のUIを改善 #1511
Update: キー割り当て設定画面のUIを改善 #1511
Conversation
<q-tr :props="tableProps"> | ||
<q-tr | ||
:props="tableProps" | ||
@click="openHotkeyDialog(tableProps.row.action)" |
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.
行全体をダイアログ表示のための当たり判定にしました
padding="none sm" | ||
size="1em" | ||
:disable="checkHotkeyReadonly(tableProps.row.action)" | ||
@click.stop=" |
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.
行全体を当たり判定にしたので、伝播しないようにstopを追加しました
hotkey === "Meta" | ||
? "Cmd" | ||
: hotkey === "" | ||
? EMPTY_COMBO_NAME |
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.
この変更により、入力状態にも見える初期状態でなぜかOKボタンが押せないという挙動になっちゃったかもです。
(以前は空欄だったのでOKボタンが押せないのは納得感があった)
今の設定値が何かわかる一方で、なぜかOkボタンを押せないという一長一短な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.
初期状態は空欄として、それとは別に下の方に小さめに『以前の設定:「Ctrl + S」』みたいに表示する感じはどうでしょうか?
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.
どちらかというとあったほうが良さそうだとは思います。
まあかなり細部なので、自転車置き場の議論っぽみがあるなという気は若干しています。
このUIに関しては↓の形が良いかもです
- 「デフォルト」は意外と難しい単語なので「初期設定」
- ショートカットキー入力は左端が丸くてどうしても縦揃えにならないから中央表示
- 「以前の設定」とかはなんとなくもうちょっと中央寄りが良さそう(だけど可変長なので難しそう)
あと、ショートカットキー入力の下に書く形であれば、今何の操作のショートカットキーを編集中なのか書いてもまあ変な伝わり方はしないのかなとちょっと思いました!
「対象の操作:ほげほげ」みたいな。(「対象の操作」という言い方はもっと良いのがありそう)
</q-chip> | ||
</template> | ||
<span v-if="lastRecord !== '' && confirmBtnEnabled"> +</span> | ||
<span v-if="isOnlyModifierKey"> +</span> |
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.
名前を変えただけで、挙動には変更ありません。
// FIXME: actionはHotkeyAction型にすべき | ||
const deleteHotkey = (action: string) => { | ||
changeHotkeySettings(action, ""); | ||
}; |
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箇所に分かれた影響で、deleteHotkeyWithConfirm
になりました。
const resetHotkey = async (action: string) => { | ||
const result = await store.dispatch("SHOW_CONFIRM_DIALOG", { | ||
title: "ショートカットキーを初期値に戻します", | ||
message: `${action}のショートカットキーを初期値に戻します。<br/>本当に戻しますか?`, | ||
html: true, | ||
actionName: "初期値に戻す", | ||
cancel: "初期値に戻さない", | ||
}); | ||
if (result === "OK") { | ||
window.electron | ||
.getDefaultHotkeySettings() | ||
.then((defaultSettings: HotkeySetting[]) => { | ||
const setting = defaultSettings.find((value) => value.action == action); | ||
if (setting) { | ||
changeHotkeySettings(action, setting.combination); | ||
} | ||
}); | ||
} |
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箇所に分かれた影響で、resetHotkeyWithConfirm
になりました。
await changeHotkeySettings(duplicatedHotkey.value.action, ""); | ||
await changeHotkeySettings(targetAction.value, targetRecord.value); |
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.
片方だけがpromiseを待たれていて不自然だったので、両方待つようにしました。
lastAction.value = action; | ||
lastRecord.value = ""; | ||
targetAction.value = action; | ||
targetRecord.value = getCurrentHotkeyCombination(action); |
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.
レビューは @y-chan さんにおまかせしたいのですが、UXに関してちょっとご相談が!
全体的にとても良い感じかなと思いました!!!ホバー中に強調表示などは思いつきませんでした、すごく良いと思います。
3点だけコメントがあります!
- デフォルトに戻す、未割り当てにする機能が、リストの中とダイアログの中の2箇所に現れていて、機能が重複しているように感じました。今後のメンテナンス性のことも考えると片方にしておきたい気持ちがあります。後述の理由で、リストの方だけにするのがいいのかもと思いました。
- こちらはこのプルリクエストで何とかしない方がいいと思うのですが、根本的な話として、マウスが使えない状況だとtabが吸われるから、ショートカットキーダイアログを閉じることができないですね・・・・・・。ちなみにVSCodeはescで閉じる、Discordはキー入力が完了すると自動的に終わるようになっていました。とりあえずダイアログの方は機能を足していけないという形になりそうなので、先ほどの提案でダイアログ内にボタンを増やすのをやめる側に倒す提案をした次第です。
- これはどこにもメモっていなかったので申し訳ないのですが、ショートカットダイアログの中に何のショートカットキーを設定しているのかのタイトルを書かなかったのは、わざとそうしています。というのもここに何が書かれるのか分からず、何か勘違いしてしまうような内容になる可能性があるので、意図的に外していました。例えば「キャンセル」などが書かれている場合に混乱を生じるかなと・・・。ちなみにVSCodeは何も書かれていませんでした。
コメントありがとうございます。 例に出してくださった Discord と VSCode のキーコンフィグを試してみました。
確かにその方が良さそうです。アイコンを使ったのにも重複してることを明示して混乱を避ける意図がありましたが、そもそもそうすれば解決ですね。
薄々気付いてました…。ちなみに VSCode は UI 上から Esc と Enter が登録不可に見えるんですが、諦めてるんですかね…?
VSCodeだと後ろの強調表示が維持されたままなので何の設定中か分かりやすいんですが、VOICEVOXだと背景が暗くなるので、入力中だというのが分かりやすいのと引き換えに何の変更中かが分かりにくくなっている気がします。 |
これはそうだと思います。実際多分クリックとかもどうしても不可能だったりするので、まあそんなもんかなと。(Esc押さないと閉じられないのは思い切った設計だな~と思いました。)
まあ確かに書いていた方が親切だなと思い直しました。説明を加える形がいいのかなと! |
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は、いままでVOICEVOX内ではなかった気がします(私の認識違いだったらすみません...)。
中心だけを見ればよく、使いやすさは上がると思うのですが、ちょっとデザインがこれでいいのかを議論しておきたいです。
例えば、ヘッダーのタイトルを中央揃えにしたとかは、今までのUIの暗黙的なルール(ルールと言うほどのものではなく、コードを書く人が勝手に守ってた決まりごとですが...)と異なるはずです。
他にも、線を端っこまでは引かずに、中央部分だけでぷっつり切るというのも今までだとあんまり見られなかったと思います。
ちょっとどうすべきかお二方(@thiramisu @Hiroshiba )の意見をお伺いしたいなと...!
レビューありがとうございます! このデザインにした理由についてとりあえず自分の思考プロセスを書いておきます。 ただ、VOICEVOXの場合は設定画面が全画面で開かれるようになっています。 デザインの統一に関してもしこのデザインで良さそうなら、他の中央揃えにした方が見やすいダイアログもこれに合わせた方が良さそうです。ただダイアログによって最適な最大横幅は変わりそうなので、そこは手動で調整が必要そうです。 線がぷっつり切れるのも確かに他の使用箇所はないんですが、そもそもq-tableが使われているのがここだけなんですよね。 あとは設定ダイアログの |
あとバグに気付きました… |
まず目的なんですが、デザインの改善とUXの改善であれば、UXの改善が優先なのかなと思ってます。
あと統一の方針ですが、設定系を1つのダイアログにまとめて、左側にドロワーを配置してそこから色々なものを設定する方針もありかも・・・? |
現状自体がこうなのですが、そもそもこれが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.
最大横幅を制限し、横長な画面でもみやすく
とりあえず両方とも左揃えにして、こんな感じになりました。
デザインに関してですが、右側が空いているUIはかなり見慣れないので、両端が空いているUIか、右側を何かしらで埋めたUIにすると良いのかなと思っています。
ツールバーは左寄せのままで良いと思います。
もし変更お願いできると嬉しいですが、難しそうであればこちらでやっちゃおうかなと思います 🙇
(個人的には真ん中寄せでとりあえず良いかなと思ってます。何がダメなのかわからなくて振動するので、とりあえず最も責任が大きい自分の考えを採用しようかなと思っています。。)
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.
これについて、根本原因は別な気がしたので、それに関してディスカッションに書き込みしました。
#1564 (comment)
自分はダイアログであるとはあまり認識できず、画面であるイメージが強かったです。
変数名ではDialogとされているにもかかわらず、自分にはダイアログっぽく見えないことについてをまずもっと考えるべきだったかもです…。
デザイン的にもそうなんですけど、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.
issue作成ありがとうございます!
ショートカットキーのテーブルを中央寄せするかどうかに関しては、ダイアログなのか画面なのかは意識してませんでした。
- 他と見た目を統一させておきたい
- キーとバリューは近く表示したい(横幅の最大長を固定にしたい)
- よく見かけるUIにしたい
を全部満たすにはまあテーブル中央寄せかなぁと。少なくとも問題は無いと思います!
そもそも若干イケてない感は僕も感じているので、どっちかというと抜本的な解決策を思いつく部分を土俵にしたい感があります。
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つ解決策を考えてみました。
- 強調表示を維持する
- 説明を加える
任意のテキストがダイアログのタイトルに表示されるのはやはりちょっと避けるべきだと思います。
極端な例ですが、将来「ダイアログを閉じる」というショートカットキーが現れた時に、OKを選べばいいのかキャンセルを選べばいいのかわからなくなります。
(説明は書いても読まれないと想定してUIを作るのがいいかなと)
強調表示を維持するのはすごく良いと思いました!!
ショートカットダイアログ内に書くのはやめて、強調表示を目指すか、あるいはまた別の機会に抜本的な解決策を考えるというのはどうでしょう。
(そもそもダイアログを開かないとか。)
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つ解決策を考えてみました。
説明を加えるまあ確かに書いていた方が親切だなと思い直しました。説明を加える形がいいのかなと!
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.
あ、大丈夫です! 🙇
二転三転してしまっていて申し訳ないです。
今回のことはかなり反省しています。
せっかくプルリクエストを頂いたんだから採用したいという気持ちと、でも自分は前のほうが良いと思っているという気持ちが両方ある中で、複数の議論が同時進行して判断基準がぶれてしまった形でした。
議論が複数起こる場合、議論内容をなるべく独立させて論点と結論が見返しやすい形を目指していこうと思います。
ガシガシUI/UXを改善できる方は経験上珍しいので、 @thiramisu さんのPRやissueはとても助かっています。
未熟者なのでご迷惑おかけしてしまうこともあるかもなのですが、これからももしよければどんどん提案頂けると嬉しいです 🙇
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.
いえ、こちらこそ、自分が甘かった故に意思決定のプロセスを乱してしまってすみません🙇
未熟で申し訳ないです。
了解です!
hotkey === "Meta" | ||
? "Cmd" | ||
: hotkey === "" | ||
? EMPTY_COMBO_NAME |
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.
この変更により、入力状態にも見える初期状態でなぜかOKボタンが押せないという挙動になっちゃったかもです。
(以前は空欄だったのでOKボタンが押せないのは納得感があった)
今の設定値が何かわかる一方で、なぜかOkボタンを押せないという一長一短な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.
ちょっと一旦実装周りはまだ見られてないのですが、GUI周りに関してコメントさせていただきました! 🙇
これはちょっと実装ではない部分の提案なのですが、複数の話が同時に続いていて、おそらくお互いに相当コミュニケーションロスが発生していると感じています。
問題はgithubのUIで、並列にコメント扱えないためだと思います。
ということでファイルにコメントを一つ一つ書いてみました。
一つ一つの話は簡単だと思うので、これで一つずつ解決できてればなと!
&:hover td::before { | ||
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示 | ||
content: ""; | ||
} |
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.
ワークアラウンドであることはコードからわからないので、メモを残しておいてあげるとリファクタリングの役に立つかもです!
こんな感じとかどうでしょう。
&:hover td::before { | |
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示 | |
content: ""; | |
} | |
&:hover td::before { | |
// Quasarデフォルトの強調表示が何故か働かないのでワークアラウンド | |
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示 | |
content: ""; | |
} |
お疲れ様です! ちょっと1年ほど時間が経ってしまい、おそらくmainブランチの追従が難しそうで、いったんcloseさせていただいても良いでしょうか 🙇 |
すみません、申し訳ないのですがcloseさせていただきます! |
内容
設定 / キー割り当て 画面のUIを改善します。具体的は以下の点です。
一覧
個別設定ダイアログ
スクリーンショット・動画など
デフォルトテーマ
ダークテーマ
横幅が足りない場合、改行は挟まるが極端には表示は崩れない(元からの仕様)
最大横幅を制限
デフォルトテーマ(OKボタン無効化中)
ダークテーマ(OKボタン無効化中)