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

Fix/change UI setting qall #4192

Merged
merged 24 commits into from
Jan 29, 2024
Merged

Fix/change UI setting qall #4192

merged 24 commits into from
Jan 29, 2024

Conversation

damin3A3
Copy link
Contributor

@damin3A3 damin3A3 commented Dec 9, 2023

issue:
#2203

設定画面の通話(Qall)のページを改善しました。
デザイン元:
https://www.figma.com/file/6Wme6N24y3GN2jD0vqZWZr/traQ-S-UI?type=design&node-id=1-1355&mode=design

変更内容
・ページの設定項目の順番の変更
・「RTC機能」「メッセージの読み上げ」の項目のトグルの位置を右側に変更
・デザイン元に従った文章、文字の太さなどの細かい変更

◯「マスターボリューム」「ノイズゲート」のスライダーですが、見た目がデザイン元通りになっているか確認できておりません。
(自分のPCはMacのため、違う見た目になっているかもしれないです。)ご確認をお願いいたします。

Copy link

github-actions bot commented Dec 9, 2023

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24d955b) 86.35% compared to head (1942bf9) 86.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4192   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          66       66           
  Lines        4719     4719           
  Branches      564      564           
=======================================
  Hits         4075     4075           
  Misses        638      638           
  Partials        6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mehm8128
Copy link
Member

mehm8128 commented Dec 9, 2023

PRありがとうございます!
プレビューでエラー出てるのはここ↓で直したのが取り込めてなさそうなので、masterブランチをmergeかrebaseしてみてください:pray:
#4158

また、issueのURL書いてくれるのはありがたいのですが、closehttps://github.com/traPtitech/traQ_S-UI/issues/2203って書いちゃうと他のページが終わってないのに設定画面のissueがcloseされてしまうので、closeは消しておいてほしいです(もしかしたら紐づけは解除されないかもなのでその場合はサイドバーから紐づけも外しておいてほしいです)

@damin3A3
Copy link
Contributor Author

damin3A3 commented Dec 9, 2023

ご確認いただきありがとうございます!
ご指摘いただいた部分を修正します。

@damin3A3 damin3A3 linked an issue Dec 9, 2023 that may be closed by this pull request
@damin3A3 damin3A3 removed a link to an issue Dec 9, 2023
@damin3A3
Copy link
Contributor Author

連絡が遅れて申し訳ありません、ご指摘いただいたところを修正しました。
お時間ある時にご確認お願いいたします。

Copy link
Member

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

大体よさそうですがいくつか書いてみました

Comment on lines +54 to +55
display: block;
font-weight: bold;
Copy link
Member

Choose a reason for hiding this comment

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

NatsukiくんのPRでFormRadioコンポーネント自体を変えてもらってるので、ここらへんはもしかしたらまたちょっと変えてもらうかもです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます!
一旦変えずに今後の指示を待ちます。

.enable {
display: flex;
align-items: center;
margin-bottom: 8px;
h3 {
margin: 0;
.header_and_content {
Copy link
Member

Choose a reason for hiding this comment

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

nits
class名はcamelCaseにしてほしいです:pray:

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。
下でアドバイスをいただいている通り、class名をsectionにしました。

@@ -2,76 +2,35 @@
<section>
<div :class="$style.element">
<div :class="$style.enable">
<h3 :class="$style.header">RTC機能を有効にする</h3>
<div :class="$style.header_and_content">
Copy link
Member

Choose a reason for hiding this comment

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

imo
多分ここはh3使ってることもあってdivじゃなくてsectionタグで囲う方がいいので、header_and_contentのclassつけてるところはdivじゃなくてsectionにして、class名もsectionにするといいと思います

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

アドバイスをありがとうございます。
header_and_contentの名前をsectionに変更して、これを使うところはdivでなくsectionタグを使うようにしました。
変更を加えなかった他の部分の見出しは元からdivですが、そこはあまり気にしなくて良いでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

他のところもできれば変えておいてほしいです!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます。わかりました。

@@ -2,76 +2,35 @@
<section>
<div :class="$style.element">
<div :class="$style.enable">
<h3 :class="$style.header">RTC機能を有効にする</h3>
<div :class="$style.header_and_content">
<h3 :class="$style.header">RTC機能</h3>
Copy link
Member

Choose a reason for hiding this comment

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

nits
元からこうだったので、できればでいいのですが、headerじゃなくてheadingが正しいと思うので余裕あったら直してほしいです(他のh3も同様)

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。
修正しました。

</div>
<div :class="$style.element">
<h3 :class="$style.header">マスターボリューム</h3>
<form-range-with-value
Copy link
Member

Choose a reason for hiding this comment

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

スライダー、僕のwindows+chromeの環境だとこんな感じなのですが、そちらではどんな表示ですか?(ブラウザも教えてほしいです)
image

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

ご確認ありがとうございます。
自分の環境だとこんな感じです。

mac+chrome モバイル表示でも大体同じ感じです。
マスターボリューム_mac+chrome

mac+safari
マスターボリューム_mac+safari

iphone+iOS PWA(Safari)
確認できなかったのですが、変更なし(開発環境でないtraQ)の場合こんな感じで、今回の変更で色などは変更していないので、これと同じ感じだと思います。
マスターボリューム_iphone+PWA

Figma上のものはchromeを想定してデザインしてくださっているのかなと思うのですが、PCやブラウザに関わらず同じデザインになるようにした方がいいのでしょうか。
よろしくお願いいたします。

Copy link
Member

Choose a reason for hiding this comment

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

なるほどです
ブラウザによって結構変わっちゃうのでこのままでもいい気がしますが、一応周りの灰色部分消すことができるかどうかくらいでいいので、調べてみてできそうだったらやってみてほしいです!

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
Contributor Author

Choose a reason for hiding this comment

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

お疲れ様です。スライダーなのですが、FormRangeWithValue.vueを変更してしまってもいいですか?

Copy link
Member

Choose a reason for hiding this comment

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

リポジトリ全体に対して<form-range-with-valueで検索をかけてみると、このコンポーネントが今のところQallの設定画面でしか使われていないことが分かるので、OKです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます。確かにQallの設定画面以外では文字列がヒットしませんでした。
では、スライダーの設定のコードを変更してみます。

</div>
<div v-if="state.isTtsEnabled" :class="$style.element">
<h3 :class="$style.header">メッセージ読み上げの声</h3>
<div :class="$style.content">
<div>
Copy link
Member

Choose a reason for hiding this comment

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

nits
このdivなくてよさそうです?

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。
確かに要らなそうだったので、消去しました。

Comment on lines 27 to 36
<form-selector
v-if="voiceOptions.length > 0"
v-model="state.voiceName"
label="読み上げボイスの種類"
:options="voiceOptions"
/>
<p v-else>読み上げ音声の声の種類が取得できませんでした</p>
<p v-else>読み上げ音声の声の種類が取得できませんでした</p>
<form-input
v-model.number="state.voicePitch"
label="ピッチ"
Copy link
Member

Choose a reason for hiding this comment

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

should
ラベルと上のフィールドの隙間がないのが気になるので、それぞれの間にmarginつけるようにしてほしいです
image

Copy link
Contributor Author

@damin3A3 damin3A3 Dec 28, 2023

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。
cssのstyleにoptionクラスを追加し、空白をつけました。
項目(「読み上げボイスの種類」など)の文字がFigmaでは少し小さくなっているので、そこも調整しました。
また、ノイズゲートの見出しとスライダの間も隙間が少し小さく感じたので、修正しました。必要なければ元に戻します。

新規クラス

  • .option:読み上げボイスの詳細設定のクラスです。上下にmarginがあり、font-sizeを小さめにしています。
  • .content:ノイズゲートのクラスです。上下にmarginがあります。
読み上げボイスオプション_フォント調節

入力デバイスなど margin修正前
ノイズゲート_修正前

入力デバイスなど margin修正後
ノイズゲート_修正後

よろしくお願いいたします。

v-model="state.voiceName"
label="読み上げボイスの種類"
:options="voiceOptions"
:class="$style.option"
Copy link
Member

Choose a reason for hiding this comment

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

Figmaで要素を選択した状態でalt押すと、他の要素との距離が見れるので、それを参考にしてちょっと値変えてほしいです:eyes:
1つ目の画像のは多分form-input自体を変えることになるのですが、変えてokだと思います
Figma_qZgZWsoOtR
Figma_WJPY1tkpEU

margin: 12px 0;
font-size: 12px;
}
.content {
Copy link
Member

Choose a reason for hiding this comment

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

ここもFigmaだと上にだけ4pxあるので、上下に12pxから変えてほしいです!
Figma_Uobqf9RdAw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

わかりました、ありがとうございます!
Figmaの方を参考に微調整します。

</div>
<div :class="$style.element">
<h3 :class="$style.header">マスターボリューム</h3>
<form-range-with-value
Copy link
Member

Choose a reason for hiding this comment

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

なるほどです
ブラウザによって結構変わっちゃうのでこのままでもいい気がしますが、一応周りの灰色部分消すことができるかどうかくらいでいいので、調べてみてできそうだったらやってみてほしいです!

@damin3A3
Copy link
Contributor Author

お疲れ様です。
要素間の空白の調整は終わったのですが、スライダの見た目の調整は少し時間がかかってしまいそうです。
締め切りが年内だったと思うので、もし一旦変更を締め切る場合はここで一区切りとさせてください。
1月も作業を続けてよさそうでしたら、引き続きスライダの調整に取り組みます。

@mehm8128
Copy link
Member

まだ自分含めて他の人も終わってないので大丈夫です!引き続きお願いします:pray:

@damin3A3
Copy link
Contributor Author

わかりました!ありがとうございます。

Copy link
Member

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

すみません最後に1つだけ:pray:

@@ -51,7 +51,7 @@ const id = randomString()
<style lang="scss" module>
.label {
@include color-ui-secondary;
Copy link
Member

Choose a reason for hiding this comment

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

ここと、FormInputのラベルの色が変わってたっぽいので直してほしいです:pray:

Suggested change
@include color-ui-secondary;
@include color-ui-primary;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。
修正しました。

@damin3A3
Copy link
Contributor Author

スライダーの見た目を変えるところに少し着手したのですが、擬似要素を使う方法があまりうまくいかず(擬似要素を使っても思ったように設定できないところがあった+できればwebkit-sliderを使わず書きたいが、擬似要素を使う場所を指定するためにwebkit-sliderが必要)、vue-slider-componentを使う方法を試してみようかと思っています。
vue-slider-componentを使う場合、設定画面のスライダについての新しい.vueファイルを書くという形になるのでしょうか?

@mehm8128
Copy link
Member

一応::-webkit-slider-thumbtransform: translateY(-4px);とかつければ表示はいい感じになりそうですが、firefoxとかだと崩れるのでwebkit-sliderは使わない方がよさそうですねー
vue-slider-componentに関しては既にASlider.vueがあるので、↓のどっちかになりそうです

  • これのCSSやpropsを変えて使う
  • 設定画面用にこれを使ったコンポーネントを新しく作る

ASliderはhttps://github.com/traPtitech/traQ_S-UI/blob/24d955b6ac3b917a950fd357634787c4fb77cc17/src/components/Main/NavigationBar/EphemeralNavigationContent/QallController/QallDetailsPanelUser.vue とかで使われているので参考になるかもです

@damin3A3
Copy link
Contributor Author

damin3A3 commented Jan 23, 2024

ありがとうございます。FormRangeWithValue.vueを書き換えて、form-inputを使っていたところをa-sliderを使用するようにしたいと思います。
ASlider.vueのデフォルト値などはなるべく変えないようにしたいと思っていますが、設定画面のスライダで新しく必要になる項目(intervalなど)は必要に応じて指定できるようにpropsに追加しました。

@damin3A3
Copy link
Contributor Author

damin3A3 commented Jan 27, 2024

スライダーの色味を変更しました。
FormRangeWithValue.vueで、スライダの棒部分の背景色の透過度を50%にするという設定で、opacityを棒だけ変更するということが出来なさそうだったため、そこだけCSS変数の参照を使わずにrgbaで設定しました。

おそらく必要な変更は一通り行ったと思います。
お時間のある時にご確認をよろしくお願いいたします。

Copy link
Member

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

大体よさそうですが、あとちょっとお願いします🙏

:max="max"
:disabled="disabled"
:tooltip="'none'"
Copy link
Member

Choose a reason for hiding this comment

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

nits

Suggested change
:tooltip="'none'"
tooltip="none"

Comment on lines +3 to +10
<a-slider
v-model.number="value"
:class="$style.range"
type="range"
:min="min"
:step="step"
:max="max"
:disabled="disabled"
:tooltip="'none'"
:interval="interval"
Copy link
Member

Choose a reason for hiding this comment

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

imo
今ちょっと縦のクリック可能範囲が狭くてクリックしづらいので、↓を入れるとかして(もっといい方法あればstyle属性入れる以外の方法がいいけど、ライブラリ側のstyle属性に上書きされちゃって難しそうかも?)クリック可能範囲を増やしてほしいかもです(デザインとちょっと余白とか変わっちゃってもOKです)
:style="{padding: '24px 0'}"

分かりづらいけど赤矢印で示した幅↓
image

Copy link
Member

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

よさそうです、ありがとうございます!(ラジオボタンの保留してたところはあとで別で対応するので一旦このままマージでOKだと思います)

@damin3A3
Copy link
Contributor Author

ありがとうございます!
マージはこちらでやってしまっていいのでしょうか?

@mehm8128
Copy link
Member

大丈夫です!お願いします

@damin3A3 damin3A3 merged commit b7a8def into master Jan 29, 2024
11 checks passed
@damin3A3 damin3A3 deleted the fix/change_UI_Setting_Qall branch January 29, 2024 12:40
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