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

設定画面プロフィールタブの改修 #4215

Merged
merged 7 commits into from
May 9, 2024

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented Jan 6, 2024

#2203
スタンプタブの改修で使ってるものを使ってるのでスタンプタブのPRから生やしてます

動作確認事項

  • 各項目を編集して更新できる
  • アイコンの変更で画像編集ができる
  • 編集して更新していない状態で別ページに移動しようとするとconfirmが出て、各選択肢に応じて正しい動作をする
  • リセットボタンを押すと変更前の状態に戻る
  • 変更がない状態、バリデーションエラーが出ている状態などでボタンがdisabledになっている

@mehm8128 mehm8128 self-assigned this Jan 6, 2024
Copy link

github-actions bot commented Jan 6, 2024

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.35%. Comparing base (d50e32d) to head (7fd6584).

Additional details and impacted files
@@                 Coverage Diff                  @@
##           feat/settings_stamp    #4215   +/-   ##
====================================================
  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.

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

アイコンの変更だけ「プロフィールを更新」を押す前に反映されるのが不自然な気がしました
何回か変えて設定画面でプレビューしたい需要がありそう
それ以外の確認項目は良いと思います

@mehm8128
Copy link
Contributor Author

mehm8128 commented Apr 1, 2024

僕もそう思ったのですが、モーダルの構造的にモーダルで加工したデータを「プロフィールを更新」を押したときの処理に渡すことができなくて、妥協してました
store使えばいけるけど、store使うほどでもないかなって思いました

@ras0q
Copy link
Member

ras0q commented Apr 1, 2024

ガチガチに共通化しててコールバック受け取れる状態じゃないですね、、、
セクション分けてるとはいえ結構非自明なのでどうにかしたい

<form-selector v-model="state.homeChannel" :options="channelOptions" />
</section>
<section :class="$style.section">
<h3 :class="$style.heading">X (旧:Twitter)</h3>
Copy link
Member

Choose a reason for hiding this comment

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

旧:Twitter ってあんまみない表記な気がします
普通に旧Twitter?

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のデザインに合わせたのですが、確かにそうかもなのでFigma含めて直します

@mehm8128
Copy link
Contributor Author

mehm8128 commented Apr 2, 2024

ガチガチに共通化しててコールバック受け取れる状態じゃないですね、、、 セクション分けてるとはいえ結構非自明なのでどうにかしたい

モーダルはhistoryでstate管理してるのをどうにかすればどうにかなりそうな気はするけど、どうすればいいかは分からないですね...

@mehm8128 mehm8128 requested a review from ras0q April 5, 2024 00:57
@ras0q
Copy link
Member

ras0q commented Apr 5, 2024

変更ボタンが更新ボタンと同じく右側にあったらちょっとは混乱減らせるかも?です

image

@ras0q
Copy link
Member

ras0q commented May 9, 2024

反映されてないかも?
image

関係あるか分からないけどCI見たらwarning出てました

#19 14.07 > traq_s-ui@3.18.2 build
#19 14.07 > vite build
#19 14.07 
#19 14.72 Browserslist: caniuse-lite is outdated. Please run:
#19 14.72   npx update-browserslist-db@latest
#19 14.72   Why you should do it regularly: https://github.com/browserslist/update-db#readme
#19 14.86 vite v4.5.0 building for production...
#19 15.06 transforming...
#19 17.27 [vite:css] end value has mixed support, consider using flex-end instead
#19 17.27 9  |  .iconContainer {
#19 17.27 10 |    display: flex;
#19 17.27 11 |    align-items: end;
#19 17.27    |     ^
#19 17.27 12 |    justify-content: space-between;
#19 17.27 13 |  }
#19 17.27 [vite:css] end value has mixed support, consider using flex-end instead
#19 17.27 25 |  .buttonContainer {
#19 17.27 26 |    display: flex;
#19 17.27 27 |    justify-content: end;
#19 17.27    |     ^
#19 17.27 28 |    gap: 16px;
#19 17.27 29 |  }

@ras0q
Copy link
Member

ras0q commented May 9, 2024

prodだと反映されてて:thinking:
とりあえずそっち見ます

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

細かいロジックは見れてないんですが大体良さそうです
レビュー確認したらマージしてOKです

</div>
</section>
<div :class="$style.section">
Copy link
Member

Choose a reason for hiding this comment

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

ここsectionじゃないのって意図的ですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

意図的じゃないので直しました

@mehm8128
Copy link
Contributor Author

mehm8128 commented May 9, 2024

prodだと反映されてて🤔

僕はdevでもスーパーリロードしたら反映されました

@mehm8128
Copy link
Contributor Author

mehm8128 commented May 9, 2024

CI通ったらマージします

@mehm8128 mehm8128 merged commit 3dfaffc into feat/settings_stamp May 9, 2024
11 checks passed
@mehm8128 mehm8128 deleted the feat/settings_profile branch May 9, 2024 11:34
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