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

エンジンの管理ダイアログのリデザイン #2255

Merged
merged 17 commits into from
Sep 5, 2024

Conversation

takusea
Copy link
Contributor

@takusea takusea commented Aug 28, 2024

内容

エンジンの管理ダイアログをリデザインします。具体的に以下を行います。

  • 必要なBaseコンポーネント(BaseToggleGroup, BaseNavigationView, BaseTextField)の追加
  • 追加したBaseコンポーネントのstoriesファイルを追加
  • BaseScrollAreaで親のheightの100%が伝搬できるように変更
    • ダイアログのフッター部の下固定をできるようにするため

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

image
image

@takusea takusea requested a review from a team as a code owner August 28, 2024 16:43
@takusea takusea requested review from Hiroshiba and removed request for a team August 28, 2024 16:43
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.

ほぼ lgtm です!!!
デザインありがとうございます、素敵だと思います!!!

いくつか細かい点をコメントさせていただきました!

src/components/Base/BaseNavigationView.vue Outdated Show resolved Hide resolved
src/components/Base/BaseScrollArea.vue Show resolved Hide resolved
src/components/Base/BaseTextField.stories.ts Outdated Show resolved Hide resolved
Comment on lines 80 to 82
[data-state="off"] > .check {
display: none;
}
Copy link
Member

Choose a reason for hiding this comment

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

チェックしたかしてないかで横の長さが変わるのは、もしかしたらユーザーが予測できないかもと思いました!
ボタングループ内のボタンが2種だけだったら問題なさそうですが、仮に3つあって左から順にボタンをクリックしようとした時、ボタンの横の長さが成長して間違えて前のボタンをクリックしてしまうかも。

アクセシビリティ的に色だけじゃなくて形も変わった方が良い、という気持ちなのかなと思いました!
良い解決策が思いついてるわけじゃないのですが、空白を表示しておくか、チェックマークが入ってもサイズが変わらない十分な横幅を確保するか、とか・・・・・・?

あるいはチェックマークを表示しないオプションがあれば良いかも!
というか問題が発生してから考えるでもいいかも!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

幅が変わることに関してはどうしても残すべきというほどのメリットはなさそうです…!
個人的に色以外でもどれが選択されてるかを示すものは欲しいと思うのでチェックマークはあるまま、幅が変わらないようにチェック時にマークの幅が増えた分padding値を減らして相殺する形にしました。

Copy link
Member

@Hiroshiba Hiroshiba Sep 3, 2024

Choose a reason for hiding this comment

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

なるほどです!一旦良さそう!!

ちょっと今すぐどうしてほしいということはないのですが、お伝えまで 🙏

ソング側でも @romot-co さんからトグルボタングループのUIが提案されていて、そちらはどちらかというと枠を追加して形を変える感じだったりします。
image

特に ソング UI のメイン画面はものすごい多機能になっていき、ボタンが大量に必要になってくるので、チェックマークを表示するスペースがもったいないので、少なくともソングUIのこのボタンに関しては、この形が良いように感じてます。
ちなみにMUIもそんな感じの見た目でした。
image
https://mui.com/joy-ui/react-radio-button/#segmented-controls

ちなみにMaterial Design 3のサンプルは選択チェックマーク式。
image
https://m3.material.io/components/segmented-buttons/overview

どちらが優れているとかはないのですが、もしかしたら将来的に見た目を揃えられるといいのかもとちょっとだけ思いました!
ただまぁアイコンがある場合は選択中のボタンに枠を設ける形に、テキストだけしかない場合はチェックマークを表示するみたいな使い分けもできると思うので、あまり気にしてないです。
(強いて言うなら、おそらくクリエイター向けソフトで何度も実行される場所のUIは、動きが無い方が好ましいかもくらい。)
こういう違いがあるよ~というのを、とりあえずお2人にお伝えしたかった次第です 🙏

border: 1px solid;
transition: background-color vars.$transition-duration;
cursor: pointer;
box-shadow: 0 1px 2px rgba(0, 0, 0, 0.05);
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)
shadow系の色、そろそろCSS変数にまとめていっちゃっても良いかもですね!

src/components/Base/BaseToggleGroup.vue Show resolved Hide resolved
Comment on lines 42 to 46
<div class="list-header">
<div class="header-title">エンジン一覧</div>
<BaseButton
label="追加"
icon="add"
Copy link
Member

Choose a reason for hiding this comment

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

ちょっと相談です!
ここの追加ボタンがデザインになじんで発見しづらいかも?とちょっと思いました!
もうちょっと目立たせられる方法があればお願いしたいかもです!

image

と思ってコメントしようとしたんですが、スクショ見た感じそうでもないかも。
このままで特に問題ない気もしてきました!!!
何か策があれば、くらいの気持ち!

Copy link
Member

@Hiroshiba Hiroshiba Sep 3, 2024

Choose a reason for hiding this comment

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

これちょっと迷ってます 😇
ちょっと不本意ですが、ここのボタンだけborderの色を濃くさせていただくかもです 🙇

案はいくつかあるのですが、それぞれちょっと問題がありそう。

  • エンジン一覧ダイアログに来た時、右側の空いてるスペースにも「エンジンを追加する」ボタンを大きく表示する
    • 一度エンジン一覧から何かのエンジンを選択するとその表示が消えちゃうので、ちょっと良くないUI。
  • エンジンサブメニューの中に「エンジンを追加」項目を追加する
    • ここ image
    • なんか気づきにくい気がしないでもない。
    • あとエンジン追加用のモーダルorダイアログコンポーネントを作らないといけない

とりあえずこのプルリクエスト的には、マージするのに障害になりきらないので、未解決で良さそう!

src/components/Dialog/EngineManageDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/EngineManageDialog.vue Outdated Show resolved Hide resolved
Comment on lines +248 to +253
<BaseButton
label="再起動"
icon="refresh"
:disabled="
uiLocked || engineStates[selectedId] === 'STARTING'
"
Copy link
Member

Choose a reason for hiding this comment

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

(判断メモです)

モーダルに入った後、最終的な操作をするボタンの横にアイコンを表示するの、良いのかどうか迷ったのですが良さそうだなと思いました!!

↓の右下のボタンのアイコンとか。
image

迷った理由は単純に、ここのボタンは任意の操作がくるので、アイコンが設定できないような複雑なものもあり、一貫性が下がると思ったためです。
一方でアイコンがあった方が絵になるし、あとこのソフトを使う人の気持ちもちょっと高まると思うので、設定できるところは設定するという方針もありだと感じました。
このトレードオフを考えて、まあスペースに余裕のあるところでアイコンもありそうなところは付けるのがいいのかなと思いました!

あ、もし何か参考にした考え方とかあったらお聞きしたいです 🙏
多分将来複雑な操作をするボタンと、アイコン設定できるボタンが入り組んだタイミングでまた迷いそうなので・・・!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

瞬時に認識できるようになるべくアイコンは入れていこうという個人的な考えでした!
まだ複雑な例にあたってないのでアイコンが設定できないものとの一貫性は考えてなかったですが、たしかにそうですね…

Copy link
Member

@Hiroshiba Hiroshiba Sep 3, 2024

Choose a reason for hiding this comment

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

別の調べてたらadobeに、ボタン内のアイコンどうするかの哲学があったので共有まで!

必要なもののみにアイコンを入れる(装飾には使わない)
https://spectrum.adobe.com/page/button/#Use-icons-only-when-necessary

優先度の高いボタンにアイコンがない場合は、優先度の低いものにはアイコンをつけない
https://spectrum.adobe.com/page/button-group/#Use-icons-only-for-the-most-critical-actions

とのことでした。真似るかどうかはさておき、参考になりそうなのでメモ。

position: absolute;
z-index: 10;
inset: 0;
z-index: 1;
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

ここのz-indexも後々全体管理できるといいかもですね!

というよりそもそもここは、エンジン追加しようとした時に右側ペイン内に追加 UI を表示して、左のドロワー部分をdisableにするために導入されてるのですが、モーダルにしちゃっても良いかもとか思いました。
あまりドロワー部分のエンジンリストが表示され続けてる意味がなさそう。

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.

ほぼLGTMです!!

#2255 (comment) のコメント追加だけお願いできると・・・!

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.

LGTM!!

素敵なデザインをありがとうございます!!!
Storybook実装もありがとうございます、レビューしやすくてめちゃくちゃ助かります!!

@Hiroshiba Hiroshiba merged commit 747f525 into VOICEVOX:main Sep 5, 2024
9 checks passed
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.

2 participants