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

新規登録関連画面の実装 #73

Merged
merged 11 commits into from
Dec 16, 2024
Merged

新規登録関連画面の実装 #73

merged 11 commits into from
Dec 16, 2024

Conversation

Eraxyso
Copy link
Collaborator

@Eraxyso Eraxyso commented Dec 12, 2024

Closes #57, #58 and #59

@Eraxyso Eraxyso marked this pull request as ready for review December 14, 2024 17:24
@mathsuky mathsuky self-requested a review December 15, 2024 02:38
Copy link
Collaborator

@mathsuky mathsuky left a comment

Choose a reason for hiding this comment

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

ありがとうございます!全体的にとてもよい実装だと思います!
いくつかレビューしましたので確認していただけると助かります:pray:

Comment on lines +23 to +25
<OAuthButton app="Github" mode="signup" class="w-full" />
<OAuthButton app="Google" mode="signup" class="w-full" />
<OAuthButton app="traQ" mode="signup" class="w-full" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

この辺りの要素はgridクラスを使用して整列してもらえると良いと思います。

Comment on lines 29 to 36
<div class="space-y-3 p-2.5">
<div class="fontstyle-ui-body-strong text-left">メールアドレスで新規登録</div>
<EmailTextbox v-model="emailAddress" placeholder="メールアドレス" />
<PrimaryButton text="次へ" class="w-full" @click="onEmailSignup" />
<div class="fontstyle-ui-caption-link text-left">
<a href="login">すでにアカウントをお持ちの場合</a>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

この辺りも同様にgridクラスを用いるのが良いと思います。

<div class="flex-1">
<PlainTextbox v-model="username" />
<div class="fontstyle-ui-caption-strong pt-1 text-text-secondary">
文字数は〇以上〇以下で、半角英数字とアンダースコアのみが使用できます。
Copy link
Collaborator

Choose a reason for hiding this comment

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

pタグとかで適当に囲ってあげた方が良いと思います。それから,僕の画面だとこの文字列が改行されてしまっているので,常に一行に収まることを保証したいです。

style="height: calc(100vh - 56px)"
>
<div class="max-w-3xl space-y-5 rounded-2xl bg-white px-14 py-10">
<div class="fontstyle-ui-title text-left">新規登録</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

このようなformのタイトル?を表すようなものは全てlabel要素で書き換え,それぞれのformとforidにより対応づけをしてください。(このドキュメントを参照してください。 https://developer.mozilla.org/ja/docs/Web/HTML/Element/label)
上記の修正をしても現状では正しくformがフォーカスされないと思いますが,それはTextboxコンポーネントの実装の問題なので別issueで作業しますl。

class="flex items-center justify-center bg-background-tertiary px-8 py-6"
style="height: calc(100vh - 56px)"
>
<div class="max-w-3xl space-y-5 rounded-2xl bg-white px-14 py-10">
Copy link
Collaborator

Choose a reason for hiding this comment

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

以下の各種ラベルとフォームもgridで整列すると良さそうです。

<div class="flex-1">
<PlainTextbox v-model="username" />
<div class="fontstyle-ui-caption-strong pt-1 text-text-secondary">
文字数は〇以上〇以下で、半角英数字とアンダースコアのみが使用できます。
Copy link
Collaborator

Choose a reason for hiding this comment

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

正規表現を使ったvalidateを実装してもらえると助かります!字数制限は仮に5以上10以下などにしてください。
また,validateルールは例えばrules.tsのようなファイルを用意してそこに記述すると再利用できて良さそうなので,可能ならそうしてください。

@mathsuky
Copy link
Collaborator

traQでしていただいた質問に対してここで回答させていただきます。

emailのtextboxではメールアドレスの形式検査が常に行われ、入力がまだ終わってないところでも「形式が正しくありません」が表示されます

これは全くその通りで,別issueで対応します。

apiの定義には/traq-oauth2/paramsがありません

これについて,traQによる認証はNeoShowcaseの機能を利用することを検討しているので,traQOAuthに関連するAPIを残すかどうかは考え中です:loading:
方針が確定したらお知らせするので,一旦そのままでお願いします。

/signup/registerではメールアドレスを?token=JWTのJWTからemailを取得するように実装しましたが、サーバーに送信する前にJWTの検証が行えませんので、不正なtokenであってもこの画面を正常に表示できます。サーバーに送信した後には検証があるから特に問題にはならないと思うけど、ページを開く際に一度サーバーと通信してJWTを検証する方がいいのかな

大半のケースにおいて正しい形式のJWTが渡されると予想されるので,毎回ページを開く際に検証する必要はない(つまり検証は最終的にそのほかのデータとともに投げるときで構わない)と個人的には考えています。

@Eraxyso
Copy link
Collaborator Author

Eraxyso commented Dec 15, 2024

レビューありがとうございます!ご指摘いただいた箇所を修正しました
パスワードについてもvalidatorを追加しましたが、文字数や具体的に使える記号を確認していただければと思います(一旦usernameとは同じように仮実装しています)

Copy link
Collaborator

@mathsuky mathsuky left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます!パスワードのバリデートについては,数字+アルファベット大文字小文字+! @ # $ % ^ & * ( ) _ + - = { } [ ] : ; " ' < > , . ? /でお願いします(後々修正する可能性があります)

<OAuthButton app="Github" mode="signup" class="w-full" />
<OAuthButton app="Google" mode="signup" class="w-full" />
<OAuthButton app="traQ" mode="signup" class="w-full" />
</div>
</div>
<div class="flex-1 pl-4">
<div class="space-y-3 p-2.5">
<div class="grid space-y-3 p-2.5">
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここが直接関係しているかはわかりませんが,メールアドレス入力フォームに文字を入れないとデザインより小さく表示されてしまっているようなので,修正をお願いします(おそらくボタンやフォームにw-[適切な幅サイズ]クラスを適用すれば良いと思います。)

@mathsuky
Copy link
Collaborator

traQOAuthについて,NeoShowcaseのユーザー認証を利用する方針で行こうと思うので,traQOAuthボタンを押したときの動作は,"/_oauth/login?redirect=/ にリダイレクトする"というように実装してください。ただ今はローカルでは動かないので,実装はコメントアウトしておいて,代わりにダミーの動作を書いておいてください。

Copy link
Collaborator

@mathsuky mathsuky 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:すぐに済むと思います

  • npm auditを実行し,問題がありそうならnpm audit fixも実行してください。

@Eraxyso
Copy link
Collaborator Author

Eraxyso commented Dec 16, 2024

ご指摘ありがとうございます
コミットしました。ご確認をお願いします

Copy link
Collaborator

@mathsuky mathsuky left a comment

Choose a reason for hiding this comment

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

LGTM ありがとうございます!

@Eraxyso Eraxyso merged commit c547874 into main Dec 16, 2024
4 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
2 participants