ユーザーセッティング画面実装 #80

Kaki256 · 2024-12-20T04:18:41Z

実装方針とかでおかしな点などあったら教えてください

close #64

mathsuky

実装ありがとうございます！ロジック面，デザイン面とも必要な事項が揃っておりとても良いと思います。

このPR内ではサイドメニューやフォーム，ボタンなどを直接実装してもらっていると思いますが，そちらについては@/components内に定義された各種コンポーネントで再定義していただければと思います。ボタンなどの配置のためのクラスはそのままに，buttonやinputなどをそのままコンポーネントで置き換えてもらえればと思います。

自分の周知不足もあり手戻りが多くなってしまって大変申し訳ございませんが，よろしくお願いします:pray:
その他少しですがコメントもしてあるのでご確認をお願いします。

mathsuky · 2024-12-20T12:42:00Z

src/views/settings/SettingsAccount.vue

+import GitHubIcon from '@/assets/service_icons/github.svg';
+import GoogleIcon from '@/assets/service_icons/google.svg';
+import traQIcon from '@/assets/service_icons/traq.svg';
+
+import checkIcon from '@/assets/status_icons/check.svg';
+import profileIcon from '@/assets/status_icons/profile.svg';
+import accountIcon from '@/assets/status_icons/account.svg';


importは先頭にまとめてしまった方が読みやすいと思います

mathsuky · 2024-12-20T12:47:38Z

src/views/settings/SettingsAccount.vue

+function selectItem(item: string) {
+  selectedItem.value = item;
+  router.push(`/settings/${item}`);
+}


これでも全く問題ないのですが，tOJ-frontの他のコードではarrowを用いた関数式の形式で関数を定義しているので，それに合わせてもらえるとありがたいです:pray:
それぞれ微妙に動作が異なりますが，今回はそのまま書き換えて問題ないと思います。

https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Functions/Arrow_functions
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Statements/function

mathsuky · 2024-12-20T13:00:07Z

src/views/settings/SettingsAccount.vue

+    <aside class="gap-3 p-3" style="min-width: 250px;">
+      <h2 class="h-10 pb-3 text-xl font-medium">設定</h2>
+      <ul>
+        <li class="mb-3">
+          <a 
+            href="/settings/account" 
+            class="flex rounded px-4 py-1"
+            style="font-weight: 500; background-color: #FFE5E5; color: #8E3535; gap: 5px;"
+            @click.prevent="selectItem('account')"
+          >
+            <img :src="accountIcon" alt="" width="20" height="20" />
+            <p>アカウント</p>
+          </a>
+        </li>
+        <li class="mb-3">
+          <a 
+            href="/settings/profile" 
+            class="flex rounded px-4 py-1"
+            style="font-weight: 500; background-color: white; color: #5F5F5F; gap: 5px;"
+            @click.prevent="selectItem('profile')"
+          >
+            <img :src="profileIcon" alt="" width="20" height="20" style="filter: invert(33%) sepia(0%) saturate(0%) hue-rotate(180deg) brightness(95%) contrast(85%);" />
+            <p>プロフィール</p>
+          </a>
+        </li>
+      </ul>
+    </aside>


これは僕が周知していなかったのが悪い部分も大きいのですが，ページ左側のメニュー/ボタン/フォームなど多くのページで共通に使われることがわかっているものはすでにコンポーネントとしてcomponents以下に定義されています。このページのメニュー，ボタン，フォームなどもそちらを使って描画していただくようにお願いいたします。少し手戻りが多くなってしまって申し訳ないです:pray_shake:

mathsuky · 2024-12-20T13:03:18Z

src/views/settings/SettingsAccount.vue

+        <div class="flex-col">
+          <label class="text-sm font-medium" for="username">ユーザー名</label>
+          <div class="flex items-center gap-2">
+            <input id="username" v-model="username" type="text" class="h-8 w-48 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />


@/components/Controls/Textbox/PlainTextbox.vueを使って再定義をお願いします:pray:

mathsuky · 2024-12-20T13:03:52Z

src/views/settings/SettingsAccount.vue

+          <label class="text-sm font-medium" for="username">ユーザー名</label>
+          <div class="flex items-center gap-2">
+            <input id="username" v-model="username" type="text" class="h-8 w-48 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+            <button class="h-10 w-20 rounded-lg" style="background-color: #AC004B; color: white;" @click="changeUsername">変更</button>


@/components/Controls/PrimaryButton.vueを使って再定義をお願いします:pray:

mathsuky · 2024-12-20T13:05:15Z

src/views/settings/SettingsAccount.vue

+        <div class="flex-col">
+          <label class="text-sm font-medium" for="email">メールアドレス</label>
+          <div class="flex items-center gap-2">
+            <input id="email" v-model="email" type="email" class="h-8 w-96 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />


@/components/Controls/Textbox/EmailTextbox.vueを使って再定義をお願いします:pray:

mathsuky · 2024-12-20T13:05:29Z

src/views/settings/SettingsAccount.vue

+          <label class="text-sm font-medium" for="email">メールアドレス</label>
+          <div class="flex items-center gap-2">
+            <input id="email" v-model="email" type="email" class="h-8 w-96 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+            <button class="h-10 w-20 rounded-lg" style="background-color: #AC004B; color: white;" @click="changeEmail">変更</button>


@/components/Controls/PrimaryButton.vueを使って再定義をお願いします:pray:

mathsuky · 2024-12-20T13:06:07Z

src/views/settings/SettingsAccount.vue

+        <h2 class="h-9 border-b-2 pb-2 text-xl font-medium" style="border-color: #D8D8D8B2;">パスワードの変更</h2>
+        <div class="flex flex-col gap-2">
+          <div class="flex-col">
+            <label class="text-sm font-medium" for="current-password">現在のパスワード</label>
+            <div class="flex items-center ">
+              <input id="current-password" v-model="currentPassword" type="password" class="h-8 w-72 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+            </div>
+          </div>
+          <div class="flex-col">
+            <label class="text-sm font-medium" for="new-password">新しいパスワード</label>
+            <div class="flex items-center ">
+              <input id="new-password" v-model="newPassword" type="password" class="h-8 w-72 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+            </div>
+          </div>
+          <div class="flex-col">
+            <label class="text-sm font-medium" for="confirm-password">新しいパスワード (確認)</label>
+            <div class="flex items-center ">
+              <input id="confirm-password" v-model="confirmPassword" type="password" class="h-8 w-72 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+            </div>
+          </div>
+          <div>
+            <button class="h-10 w-20 rounded-lg" style="background-color: #AC004B; color: white;" @click="changePassword">
+              変更
+            </button>
+          </div>
+        </div>


@/components/Controls/Textbox/PasswordTextbox.vueと@/components/Controls/PrimaryButton.vueを使って再定義をお願いします:pray:

mathsuky · 2024-12-20T13:22:44Z

src/views/settings/SettingsAccount.vue

+const services = ref<Service[]>([
+  { name: 'GitHub', linked: false, ID: '', icon: GitHubIcon },
+  { name: 'Google', linked: false, ID: '', icon: GoogleIcon },
+  { name: 'traQ', linked: false, ID: '@test_user', icon: traQIcon },
+]);


ここの値のlinkedについては，ページのマウント時に/users/meを叩いて，そのときの値を元にlinkされているか決めたいですね。
一旦，GitHubとtraQのlinkedについてはそれぞれ/users/meのレスポンスのgithubIdとtraqIdがnullかどうかで判定するようにしてみてください。(Googleは連携しているかどうか判定できるプロパティがなかったので別issueに直しておきます。)

参照; https://github.com/traP-jp/traO-Judge-docs/blob/develop/api/backend/frontend-backend.yaml

mathsuky · 2024-12-20T13:47:57Z

src/views/settings/SettingsAccount.vue

+import { useRouter } from 'vue-router';
+
+const router = useRouter();
+const selectedItem = ref<string>(window.location.pathname.split('/').pop() || 'account');


このページが呼び出されるのは/settings/accountだけなのでわざわざパスからとってこなくても良い気がします。(Profileの方も同様に。)

import を先頭にまとめる

Kaki256 · 2024-12-28T14:50:30Z

コメントの内容反映しました (selectedItem あたりに関しては SideMenuUserSetting.vue に置き換えた段階で不要になったので消えてます)

懸念点

fetchUserData() の実装がいまいちわかってない
- User 型がどこかで定義されていたりしますか
- モックサーバー立てて 'http://localhost:4010/users/me' にアクセスすることで情報を取れることは確認しましたが、本番用のリンクがこれでいいのかわからない

mathsuky

かなり手戻りが多くなってしまったにも関わらずさっと実装してくださりありがとうございます！レビュー遅れて申し訳ないです。

スタイル周りほとんど完璧です，ありがとうございます！少しだけコメントしてあるので確認お願いいたします。
またAPI周りについて，いただいた質問に対する返答はレビュー中に含めてあるので合わせて確認いただき，実装の修正をお願いできればと思います。

連絡不足で往復が増えてしまって申し訳ないですが，これで最後になると思うのでよろしくお願いいたします:pray:

mathsuky · 2025-01-02T12:12:43Z

src/views/settings/SettingsAccount.vue

          <div class="flex-col">
            <label class="text-sm font-medium" for="current-password">現在のパスワード</label>
            <div class="flex items-center ">
-              <input id="current-password" v-model="currentPassword" type="password" class="h-8 w-72 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
+              <PasswordTextbox v-model="currentPassword" />
            </div>
          </div>


この辺りのlabelとtextboxの塊のところは一つネストを少なくして良さそうです。例えば下のように

<label class="text-sm font-medium" for="current-password">現在のパスワード</label> <PasswordTextbox v-model="currentPassword" autocomplete="current-password" id="current-password" class="w-64" />

他の類似部分もネストを減らしてもらえると良いと思います！

mathsuky · 2025-01-02T12:17:44Z

src/views/settings/SettingsAccount.vue

-            <input id="username" v-model="username" type="text" class="h-8 w-48 rounded border" style="border-color: #D8D8D8; padding-left: 10px; color: #3A3A3A;" />
-            <button class="h-10 w-20 rounded-lg" style="background-color: #AC004B; color: white;" @click="changeUsername">変更</button>
+            <PlainTextbox v-model="username" />
+            <PrimaryButton text="変更" @click="changeUsername" />
          </div>
        </div>


この辺りはネストを一つ落とせそうです。例えば下のように

<label class="text-sm font-medium" for="username">ユーザー名</label> <div class="flex items-center gap-2"> <PlainTextbox id="username" v-model="username" /> <PrimaryButton text="変更" @click="changeUsername" /> </div>

他の類似部分もネストを減らしてもらえると良いと思います！
また，Textboxに対してforと対応づくidを指定してもらえると助かります:pray:
今まではコンポーネントの実装に抜けがあったのですが，修正PRがマージされているので，最新のmainブランチを取り込んでからidの指定をお願いいたします。(これは他すべてのTextbox系コンポーネントについても同様です。)

mathsuky · 2025-01-02T12:38:39Z

src/views/settings/SettingsAccount.vue

+  console.log('TODO: パスワードの変更を反映する');
+}
+
+async function fetchUserData() {


この辺り自前実装してくださってありがとうございます！ただ，APIの関数は基本的に@/api/generated/apis/以下に定義してある自動生成の関数がありますので，こちらを利用する形でお願いいたします:pray:
API定義周りで若干の変更が入っているので，mainの変更をマージしてもらってから作業してもらえると良いと思います。

合わせて，コメントの方でいただいていた質問に回答しておきます。

User 型がどこかで定義されていたりしますか

これは@/api/generated/modelsで定義されています。(このコードであれば明示的にUser型を宣言せずともgetMeを呼び出すだけで事足りるとは思います。)

モックサーバー立てて 'http://localhost:4010/users/me' にアクセスすることで情報を取れることは確認しましたが、本番用のリンクがこれでいいのかわからない

本番用のリンクはまだ確定していないです:gomen:前述した通り，自動生成の関数を利用すれば自動的にベースが設定されますので，このコードを書く上では気にしなくても問題ないです。

mathsuky · 2025-01-02T12:39:37Z

src/views/settings/SettingsAccount.vue

+      </div>
+      <div class="flex flex-col gap-3 pb-3">
+        <h2 class="h-9 border-b-2 pb-2 text-xl font-medium" style="border-color: #D8D8D8B2;">外部サービスとの連携</h2>
+        <div>


このdivはなくても支障がなさそうですので，削ってネストを浅くしてください

mathsuky · 2025-01-02T12:40:45Z

src/views/settings/SettingsAccount.vue

+    <SideMenuUserSetting />
+    <div class="flex flex-col gap-6 p-3" style="width: 800px;">
+      <div class="flex flex-col gap-3 pb-3">
+        <h2 class="h-9 border-b-2 pb-2 text-xl font-medium" style="border-color: #D8D8D8B2;">基本情報</h2>


この部分のカラーコードはtailwind.config.jsに定義してあるので，そちらのカスタムクラスを利用するようにしてください！(他のカラーコードが直書きされている部分も同様に修正をお願いします。)

mathsuky · 2025-01-02T12:41:12Z

src/views/settings/SettingsAccount.vue


 <template>
-  <div>
-    <h2>Settings Account</h2>
+  <div class="flex gap-12 px-6 py-8" style="font-family: 'Open Sans', 'Noto Sans', sans-serif;">


この部分のフォントスタイルはtailwind.config.jsに定義してあるので，そちらのカスタムクラスを利用するようにしてください！

mathsuky · 2025-01-02T12:46:27Z

src/views/settings/SettingsAccount.vue

+          <div class="flex-col">
+            <label class="text-sm font-medium" for="current-password">現在のパスワード</label>
+            <div class="flex items-center ">
+              <PasswordTextbox v-model="currentPassword" autocomplete="current-password" />
+            </div>
+          </div>


この部分も前述のようにネストが減らせそうなので，例えば以下のようにしてネストを減らしてもらえると助かります

<label class="text-sm font-medium" for="current-password">現在のパスワード</label> <PasswordTextbox v-model="currentPassword" id="current-password" autocomplete="current-password" class="w-64" />

他の類似部分も同様に修正をお願いいたします

…na641/#64

Kaki256 added 7 commits December 17, 2024 22:14

仮作成

317d19b

🍲 見た目の修正 + アイコンの追加

c7a2ea8

🍇 traQ アイコン

e588edb

🍩 traQ アイコンを入れる

8102cb3

🌮 アイコンを追加

e548c42

アイコンを追加 + 見た目の調整

a0cf8ad

☕ SettingsView.vue から Settings を削除

cd83243

mathsuky self-requested a review December 20, 2024 11:23

mathsuky reviewed Dec 20, 2024

View reviewed changes

Kaki256 added 5 commits December 28, 2024 18:41

🍱 コンポーネントを使う

d84aa85

import を先頭にまとめる

🍫 必要ない svg ファイルを削除

b7da2ce

SettingsProfile 修正

fb8a282

fetchUserData

fd3c32e

🥯 ID のケースが違いました～

376d954

mathsuky self-requested a review December 31, 2024 05:28

mathsuky reviewed Jan 2, 2025

View reviewed changes

Merge branch 'main' of github.com:traP-jp/traO-Judge-frontend into Pi…

4b0a17e

…na641/#64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ユーザーセッティング画面実装 #80

ユーザーセッティング画面実装 #80

Kaki256 commented Dec 20, 2024

mathsuky left a comment •

edited

Loading

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

mathsuky Dec 20, 2024

Kaki256 commented Dec 28, 2024

mathsuky left a comment •

edited

Loading

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

mathsuky Jan 2, 2025

ユーザーセッティング画面実装 #80

Are you sure you want to change the base?

ユーザーセッティング画面実装 #80

Conversation

Kaki256 commented Dec 20, 2024

mathsuky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kaki256 commented Dec 28, 2024

mathsuky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathsuky left a comment •

edited

Loading

mathsuky left a comment •

edited

Loading