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

ユーザー個別ページ > 回答一覧をVue.js化した #4319

Merged
merged 25 commits into from
Apr 5, 2022

Conversation

eatplaynap
Copy link
Contributor

@eatplaynap eatplaynap commented Feb 28, 2022

Issue概要

Issueのdescriptionに書いてある通り、下記3点を修正しテストを追加しました。

  • 回答一覧 を vue.js化
  • ページのタイトルを変更(user名の回答user名)
  • ページャーを付ける

参考にしたIssue

変更後

Image from Gyazo

確認手順

  1. ブランチ feature/convert-users-answers-to-vuejsをローカルに取り込む
  2. bin/setupを実行
  3. rails sでサーバーを立ち上げる
    4. rails cでコンソールを開き、下記手順で回答データを作成
  4. bin/rails db:seedでfixtureデータを作成する
  5. komagataでログイン後、ユーザー個別ページから回答タブをクリックし、Vue化された回答一覧画面を確認する

※4でrails cを行っている理由は、#4438 でのバグでdb/fixtures/answers.ymlが機能していないことが発覚し、rails db:seedでfixtureが作れないためです。
こちらも私の方で対応予定ですが、こちらのバグが解消されmainに反映されるまでは、お手数ですが上記手順でデータを作成いただき、ページャーのご確認をお願いいたします:pray:

2022/3/23 #4447 での修正がマージされたのでfixtureを作成しました。

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch 3 times, most recently from 5a18fff to 35a24ad Compare March 18, 2022 07:08
@eatplaynap eatplaynap changed the title 一旦コミット(表示されてない) ユーザー個別ページ > 回答一覧をVue.js化した Mar 18, 2022
@eatplaynap eatplaynap self-assigned this Mar 18, 2022
@eatplaynap eatplaynap marked this pull request as ready for review March 18, 2022 14:51
@eatplaynap eatplaynap requested a review from aim2bpg March 18, 2022 14:51
@eatplaynap
Copy link
Contributor Author

@aim2bpg
お疲れさまです。だいぶ時間が経ってしまいましたが、ペアプロしていただいたVue化のIssueのPRです。
お手すきの際にレビューをお願いいたします:pray:

@aim2bpg
Copy link
Contributor

aim2bpg commented Mar 18, 2022

@eatplaynap さん、レビュー依頼ありがとうございます。
本日中には確認して、結果をお知らせできればと思います〜😄

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、動作確認していたらコンフリクトが起きていることに気付きました。
まずは、手元で下記を実行してコンフリクトを解消されてください🙏

$ git pull --rebase origin main

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch from 39758da to 2bbdb15 Compare March 19, 2022 08:44
@eatplaynap
Copy link
Contributor Author

@aim2bpg
ご確認ありがとうございます!コンフリクト解消させたので再度ご確認お願いします〜

@aim2bpg aim2bpg self-requested a review March 20, 2022 09:34
Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、コンフリクト解消ありがとうございます😄

確認したところ、HTMLのVue.js化でインデントがずれていたり、classが違っていたりしますので、デベロッパーツールで対応をとりながら確認した方が良さそうです。今回.vueファイルを新規作成しており、間違っていても今後なかなか気付きにくい部分ですので、今後のためにも今回きちんと合わせ込みしておいた方が良いと思いました(ここは他の機能でもバグがあったので、間違いやすい部分なのかなと個人的には思っています😅 )。

こちらで確認した際の画面を貼っておきますのでご参考ください。
左がステージング環境(Vue.js化前)、右が開発環境(Vue.js化後)です。

image

usersAnswer: UsersAnswer
},
props: {
usersPath: { type: String, default: '', required: false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
usersPath: { type: String, default: '', required: false }
usersPath: { type: String, default: null, required: false }

ここのdefault値はnullとした方が良いようです。
理由は、#4293 (comment) を参照ください。

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

すみません、もう一点ありました。。
default値の設定見直しもお願いいたします🙏

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch from 2bbdb15 to fc9fbab Compare March 21, 2022 07:19
@eatplaynap eatplaynap requested a review from aim2bpg March 21, 2022 07:46
@eatplaynap
Copy link
Contributor Author

@aim2bpg
ご確認ありがとうございます!ご指摘の点に加えて、実装に漏れがあった分を修正したので再確認をお願いいたします:pray:

  • Vue.js化後のHTMLとVue.js化前に揃えるよう修正
  • default値の設定を修正
  • 質問の更新日付を表示させるよう修正(:new:)

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、確認しました。

  • もう1箇所変換できてないところがありました👀(下記のスクショ参照、左:ステージング環境、右:開発環境)。
  • 今回、目視で気付けたところは再発防止の観点でテストを入れておきましょう💪
    バグ関連の修正はテストもセットと考えておいた方が良いと思います(駒形さんから過去にご指摘受けました)。
  • 今回、Vue.js化したことで不要となるファイルやコードはなかったでしょうか。もし、不要であれば削除しておきましょう✨
    ex. /app/views/users/answers/_answer.html.slim, /app/controllers/users/answers_controller.rbの@answers

image

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch from 629d777 to 3eec19a Compare March 22, 2022 05:29
@eatplaynap
Copy link
Contributor Author

@aim2bpg
丁寧なレビューありがとうございます!ご指摘いただいた下記3点の対応を行いましたのでお手すきの際ご確認お願いします:pray:

  • 各回答の名前がユーザー個別ページへのリンクになるように修正
  • ↑のテスト追加
  • Vue化に伴い使用しなくなった回答一覧関連のデータを削除

@eatplaynap eatplaynap requested a review from aim2bpg March 22, 2022 06:08
Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、確認しました。
さきほどは気付かなかったのですが、回答日時が変換前(左)が更新日時に対して、変換後(右)が作成日時となっているようです。
image

あと、question.vueが書き変わっていたのが原因不明というのが気持ち悪い感じがしています。
この辺でRebase時のコンフリクト解消か何かで書き変わっていたりしないでしょうか?おそらく、Files changedに現れない差分かなと思って、レビューできているか心配してます🤔
もし、単純に1箇所だけ誤って書き換えてた〜とかで、クリアとなっていれば余計な心配かもしれません🙏

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch from 5063c04 to e89b551 Compare March 23, 2022 05:14
@eatplaynap eatplaynap requested a review from aim2bpg March 23, 2022 05:17
@eatplaynap
Copy link
Contributor Author

@aim2bpg
自分では細かいところを見落としがちなので、たくさん見つけてくださってありがとうございます〜:sob:

回答一覧の日付について

ステージング環境の回答一覧の日付を自分の実装したものと見比べたところ、そもそも仕様を勘違いしていたことに気づきました。

修正前: 回答に紐づく質問の作成日時が表示される
修正後(ステージング環境と同じ): 各回答の更新日時が表示される

上記の変更に伴い、jbuilderで渡していたanswer.question.updated_atも必要なくなったので削除しました。

question.vueの変更について

これは参考にしようと思って触っているうちに書き換えてしまったという認識です。

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、修正ありがとうございます。question.vueの変更についても了解しました👍
下記の1点を修正いただければ、 私はOKです🙆‍♀️チームリーダーレビューに進められてください👍

@garammasala29 さんの方でVue.js化された質問ページの方のselectedTagのdefault値の設定を修正しておきましょう。
本PRのスコープ外となりますが、動作確認中にコンソールにワーニングが出るのが気になりましたもので😅
→ こちらは、開発MTGでスコープ外とのことでしたので、私の方でIssue #4473 に登録しました🙏
@garammasala29 さんの修正とも直接関係なく、元々内在したものでした。早合点して失礼しました🙇🏻‍♂️

@eatplaynap eatplaynap requested a review from aim2bpg March 23, 2022 09:48
@eatplaynap
Copy link
Contributor Author

@aim2bpg
度々のご確認ありがとうございます〜!
#4447 の変更がマージされ、answers.ymlのfixtureが作成できるようになったのでもう一度レビューをお願いいたします:pray:

@eatplaynap eatplaynap force-pushed the feature/convert-users-answers-to-vuejs branch from 47b5060 to 539f2e9 Compare March 30, 2022 07:12
@eatplaynap
Copy link
Contributor Author

@komagata
お疲れさまです。AnswerのAPIにユーザーIDでの絞り込み機能を追加する形に実装しなおしたので再度レビューをお願いいたします!

@eatplaynap eatplaynap requested a review from komagata April 1, 2022 08:11
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認しました、OKですー🙆‍♂️

@komagata komagata merged commit 7eccf17 into main Apr 5, 2022
@komagata komagata deleted the feature/convert-users-answers-to-vuejs branch April 5, 2022 12:20
@github-actions github-actions bot mentioned this pull request Apr 5, 2022
11 tasks
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.

3 participants