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.

Sign up for GitHub

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

Jump to bottom

ヘルプ画面のライセンス情報セクションのリデザイン #1650

Merged
merged 19 commits into from
Dec 18, 2023
Merged

ヘルプ画面のライセンス情報セクションのリデザイン #1650

merged 19 commits into from
Dec 18, 2023

Conversation

takusea
Copy link
Contributor

@takusea takusea commented Nov 15, 2023

内容

ヘルプ画面のライセンス情報セクションの新しいデザインへの変更と、それに伴う必要なコンポーネント等の追加を行います。具体的には以下を行います。

  • 必要なベースコンポーネントの追加（BaseButton, BaseRowCard）
  • paddingやborder-radiusをコンポーネント間で統一するためのSCSS変数の追加
  • リデザインしたライセンス情報セクションのコンポーネントの追加
  • HelpDialogでインポートするコンポーネントを新デザインの方に変更

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

image
image

その他

  • 新デザインはQuasarのデザインと乖離しており打ち消すコードで複雑化しないために、極力Quasarへ依存しないよう進める方針です。
  • 色変数は変更による影響が及ぶ範囲がわからないため、暫定でコンポーネントにHex値をハードコーディングしてます。

@takusea takusea requested a review from a team as a code owner November 15, 2023 16:25
@takusea takusea requested review from Hiroshiba and removed request for a team November 15, 2023 16:25
Hiroshiba
Hiroshiba reviewed Nov 15, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

PRありがとうございます！！！
アクセシビリティの考慮に関して抜けてたのでコメントです！！ 🙇

  1. アクセシビリティ

quasar使わない場合、アクセシビリティ対応がたぶんかなり大変かなと思います。
ちゃんと知らないんですが、WCAGが定めている各コンポーネントの挙動がいろいろあるんですよね。例えば今回のPRと関係ないけど、ドロップダウンリストは上下キーでメニュー移動できてenterとspaceで決定できるべき、みたいな。

アクセシビリティを考えると既存のライブラリに乗っからないと厳しいのかなと思ってます。（自作するならこんな感じのことを気をつけないといけない）
その場合手は２つあって、quasarのデザインを打ち消すか（大変）、デザインレスのコンポーネントフレームワークを使うか（メンテされるかが見通せないので将来大変かも）かなと。
unstyledなフレームワークは例えばPrimeVueやHeadless UIが良さそう･･･？まだデファクトスタンダードができていない状態なのかなと思っています。

  1. デザインプロジェクトをまとめるポータルissue

もう１点、リデザインをまとめるポータル的なissueがあるとあとあと便利そうかなと思いました！
いつもはよくvoicevox_projectリポジトリのissueに「プロジェクト」としてissue化しています。
今思いついてるタスクのリストアップをして、完了し次第チェックつけたり、思いついたら追加したり、あとは議論を残せる場にするのが良いのかなと！
この辺りが参考になると思います。

お時間ある時で大丈夫なので制作頂けると･･･！ 🙇

@takusea
Copy link
Contributor Author

takusea commented Nov 15, 2023

承知しました！確かにポータルissue作るべきでした、とりあえずそちら作成します！

@sevenc-nanashi
Copy link
Member

個人的に：このリポに「project-new-ui」みたいなの生やしてそのブランチにPRしてくべきな気がしました。

@Hiroshiba
Copy link
Member

ブランチ戦略はどうしていくのが良さそうか結構迷ってます･･･！ 😇
広範囲の変更になってコンフリクトが何度も起こりそうだから逐一mainブランチにマージしたい気持ちもあるし、（ヘルプウィンドウ内に）デザインが見た目上２種類ある状態になるから別ブランチにしたい気持ちもあるし、という感じです･･･。

@y-chan さんや @takusea さん的にはどうでしょう 👀
個人的には････････うーーーん、設定ダイアログのりデザインが完了するかmainブランチがバージョン0.15アプデするまで、別ブランチにマージしていく形が良いかなと思いました！！
まあそもそもヘルプ周りは変更があまり無いでしょう、という読みで･･････。

@takusea
Copy link
Contributor Author

takusea commented Nov 16, 2023

先にButtonなどのベースのコンポーネントやSCSS変数のなどリデザインに必要なパーツを揃えておくのもアリかなと思いました。変更ではなく追加系の作業なので他とのコンフリクトが起きにくく、その後の作業もより速く進められて他の作業との差分も生まれにくくなるかなと！

その後の作業は各画面（ダイアログ）単位でブランチを切ってリデザインを行っていくのが一つの折衷案かなと思いました。いちいちブランチを切る手間はかかりますが…

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 20, 2023 •
edited
Loading

すみません遅くなりました 🙇

先にButtonなどのベースのコンポーネントやSCSS変数のなどリデザインに必要なパーツを揃えておく

確かにそれもありだなと思いました！

まあでもそこはコンフリクトが起こりにくいので、各画面の実装で必要になるたびに追加でもいいのかなと思いました。
見た目もチェックできますし、実際に使う側を実装しないとベース側もわからないことがよくあるので、たぶん一緒のブランチで開発する方が楽だと思います！

各画面（ダイアログ）単位でブランチを切ってリデザインを行っていくのが一つの折衷案

良さそうに思いました！！
もしコンフリクトが発生しまくったりしたら運用再検討してもいいかもですね！
最初はヘルプダイアログ周り（project-redisign-help-dialog）とかでしょうか？ 👀

（追記）名前変えるのはすぐできるので、とりあえずそのブランチ名で作ってみました！！project-redisign-help-dialog

@takusea takusea changed the base branch from main to project-redisign-help-dialog November 20, 2023 13:50
@takusea
Copy link
Contributor Author

takusea commented Nov 20, 2023

確かにレビューのしやすさの観点だと画面に実際に組み込まれたのを確認したほうがわかりやすいですね…そちらの方針でお願いします！
ブランチ作成ありがとうございます！このプルリクエストのマージ先をproject-redisign-help-dialogに切り替えました。

@Hiroshiba
Copy link
Member

切り替えありがとうございます！
あと残る課題はこちらのコメントのアクセシビリティをどうするかでしょうか･･････。
（リデザイン全体の方針に関わることなのでポータルissueの方で議論した方がいいかも･･･？そっちの方がよければコメント転記します！）

@takusea
Copy link
Contributor Author

takusea commented Nov 21, 2023

ここ以外のところで議論する形が情報がまとまって良さそうだと思います！個別issue作ろうかとも考えましたが、どのくらい長く議論するかもわからないので、とりあえずポータルissueの方に移して長引きそうだったら個別に作る感じかなと

@Hiroshiba Hiroshiba mentioned this pull request Nov 21, 2023
Open
41 tasks
@takusea
Copy link
Contributor Author

takusea commented Dec 10, 2023 •
edited
Loading

  • ScrollBarのスタイル等実装を、BaseScrollAreaとしてRadix Vueを使用したものに入れ替えました
  • Focus時のOutlineのスタイルや見出しのスタイルをSassのmixinとして定義しました

@takusea takusea closed this Dec 10, 2023
@takusea takusea reopened this Dec 10, 2023
Hiroshiba
Hiroshiba reviewed Dec 12, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ほぼLGTMです！！！！
すごくいいですね！！！！

ちょっと今後の方針にも関与してくるので、最初ならではの相談がいくつかあります！！

あ、ちょっとお伝えし忘れてたのですが、project-ブランチているにしている関係で、開発ブランチにmainをマージするとそれが差分になってしまうという課題があります！
レビューする時にどっちがどっちの差分かわからなくなるので、もしよかったらプルリクエスト用のブランチにmainをマージするのを避けつつ、mainをまあでするだけのプルリクエストを分けてproject-ブランチに送っていただければと･･･！ 🙇
（ドキュメント化できてなくてすみません 🙇 ）

src/styles/mixin.scss Outdated Show resolved Hide resolved
src/styles/variables.scss Outdated Show resolved Hide resolved
src/components/help/HelpDialog.vue
Comment on lines +89 to -92
import OssLicense from "../template/OssLicenseSection.vue";
import HelpPolicy from "./HelpPolicy.vue";
import LibraryPolicy from "./LibraryPolicy.vue";
import HowToUse from "./HowToUse.vue";
import OssLicense from "./OssLicense.vue";
Copy link
Member

Choose a reason for hiding this comment

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

あ、以前のvueファイルはデリートでもいいかもです！
（消し忘れが発生しそう）

src/components/template/OssLicenseSection.vue Outdated
@use '@/styles/mixin' as mixin;

.container {
// 親コンポーネントからheightを取得できないため一時的にcalcを使用、HelpDialogの構造を再設計後100%に変更する
Copy link
Member

Choose a reason for hiding this comment

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

このコメント良いですね！！
FIXMEかTODO付けても良いかも？（まあ気分ぐらいの違いしかないですが）

src/components/base/BaseButton.vue Show resolved Hide resolved
src/components/base/BaseScrollArea.vue Outdated Show resolved Hide resolved
@takusea
Copy link
Contributor Author

takusea commented Dec 12, 2023

ありがとうございます！

MainブランチからマージしたときにFile Changedが増えててしくじったなと思ってました…すみません、気をつけます！

@takusea
Copy link
Contributor Author

takusea commented Dec 16, 2023

  • Baseクラス名がRadix Vueのコンポーネント名と極力一対一になるように変更しました。
  • q-iconやTODOの文言など、コメントを追加しました。
  • 古い方のコンポーネントを削除しました。
  • padding,gap,radiusのSass変数名をpadding-1,padding-2などのような形式に変更しました。
    • padding-basis等の基準値を表すCSS変数を作り、そこから何倍かを表しています。

Hiroshiba
Hiroshiba approved these changes Dec 17, 2023
View reviewed changes
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！！！！！
お疲れ様でした！！！これから楽しみですね！！！

あ、１点だけthumb周りのクラス名についてコメントさせていただきました！
まあ今後いろいろ実装していってみて変えていく、とかでもありかなとも思います！
そこだけ方針のコメント返信いただければマージさせていただこうと思います！！

src/components/base/BaseScrollArea.vue Outdated Show resolved Hide resolved
@takusea
Copy link
Contributor Author

takusea commented Dec 17, 2023

ScrollAreaThumb > ScrollAreaThumbInnerにしようとしてしっくりこなかったのでその形になってましたが、たしかに言われてみればdiv要素にまでパスカルケース的な命名にする必要なかったですね。とりあえずそちらの提案した方向性でいきたいと思います！ということで該当箇所に変更入れました。

@Hiroshiba
Copy link
Member

変更ありがとうございます！！マージします！！！！！

ヘルプダイアログデザイン用にブランチ分けているので、じゃんじゃん変更していって問題ないと思います！
良い形を見つけていきたいですね･･･！！

@Hiroshiba Hiroshiba merged commit 052cf56 into VOICEVOX:project-redisign-help-dialog Dec 18, 2023
7 checks passed
@takusea takusea deleted the redesign-help-osslicense-section branch February 21, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@takusea @sevenc-nanashi @Hiroshiba