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.

Sign up for GitHub

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

Jump to bottom

ユーザー個別ページ > 質問一覧 の vue.js化 #4343

Merged
merged 13 commits into from
Mar 18, 2022
Merged

ユーザー個別ページ > 質問一覧 の vue.js化 #4343

merged 13 commits into from
Mar 18, 2022

Conversation

garammasala29
Copy link
Contributor

@garammasala29 garammasala29 commented Mar 3, 2022 •
edited
Loading

Issue

概要

ユーザー個別ページの質問一覧(ページャー含む)をVue.js化させました。
#4293、#3181 のPRを参考にしています。
詳細は以下の日報にまとめました。
Day302 [システム開発] APIを叩いてVue化させる | FJORD BOOT CAMP（フィヨルドブートキャンプ）

変更確認方法

  1. ブランチ feature/convert-users-questions-list-to-vuejsをローカルに取り込む
  2. bin/setupとrails db:seedを実行しDBの内容を開発環境に反映する
  3. rails s でローカル環境を立ち上げる
  4. ユーザー個別ページの質問タブをクリックすると、今回の変更箇所が確認できます。
    (http://localhost:3000/users/253826460/questions だとページャーも確認しやすいと思います。)

変更後

default.mov

@garammasala29 garammasala29 marked this pull request as ready for review March 4, 2022 01:29
@garammasala29 garammasala29 self-assigned this Mar 4, 2022
@garammasala29
Copy link
Contributor Author

@aim2bpg さん
お疲れ様です。ペアプロさせていただきましたissueです。
お手隙の際にご確認よろしくお願いします🙇‍♂️
ご不明点などありましたら、何なりとお申し付けください。

@garammasala29 garammasala29 requested a review from aim2bpg March 4, 2022 01:34
aim2bpg
aim2bpg reviewed Mar 4, 2022
View reviewed changes
app/javascript/questions.vue Outdated
Comment on lines 34 to 38
props: {
emptyMessage: { type: String, required: true },
selectedTag: { type: String, required: true }
selectedTag: { type: String, required: true },
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.

[問題なし]ご参考までに、37行目を一番上か36行目の上に持ってくると、36行目の2行の差分がなくなります😄

aim2bpg
aim2bpg approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@aim2bpg aim2bpg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@garammasala29 さん、先日はペアプロありがとうございました🙇🏻‍♂️
私はOKです🙆‍♀️　チームリーダーレビューに進められてください〜

動作確認中に下記のバグが内在していることが分かりましたが、別Issueを立てて対応いただければ良いかと思います。ご確認ください。

image

(推測)
Vue.js化前のプラクティス > 質問一覧の方はリンクできていますので、おそらく、最初のMenu > 質問一覧をVue.js化した際に、本Issueで変更してない共通部品のquestion.vueの中にリンクの変換ミスがあったのではないかと推測してます(間違ってたらすみません🙏 )。修正時は再発防止の意味でテストを入れた方が良さそうです。レビューの際は遠慮なく申し付けください😄

@aim2bpg
Copy link
Contributor

aim2bpg commented Mar 4, 2022

@garammasala29 さん、たびたびすみません🙏
上記のバグは、@Paru871 さん #4157 (comment) の方で修正中かもですので、一応、お知らせしておきます🙇🏻‍♂️

@garammasala29
Copy link
Contributor Author

@aim2bpg さん
お忙しい中、早速ご確認いただきありがとうございました！
バグの件ですが、確認いたしました。連絡が遅くなりすみません。その後の経過まで追っていただき感謝です🙏

@garammasala29 garammasala29 requested a review from komagata March 4, 2022 12:35
@garammasala29
Copy link
Contributor Author

@komagata さん
お疲れ様です。お手隙の際にこちらのレビューをお願いいたします🙇‍♂️

komagata
komagata reviewed Mar 7, 2022
View reviewed changes
app/controllers/api/users/questions_controller.rb Outdated
@@ -0,0 +1,13 @@
# frozen_string_literal: true

class API::Users::QuestionsController < API::BaseController
Copy link
Member

Choose a reason for hiding this comment

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

こちら、複数のAPIを作るよりも、QuestionのAPIにユーザーIDでの絞り込み機能をつけることで、対応できればと思います。

https://github.com/fjordllc/bootcamp/blob/main/app/controllers/api/questions_controller.rb#L3

/api/quesions?user_id=1234みたいなURLでアクセスするイメージです。

@garammasala29 garammasala29 force-pushed the feature/convert-users-questions-list-to-vuejs branch from a4008bd to 3096ba3 Compare March 13, 2022 23:03
@garammasala29
Copy link
Contributor Author

@komagata さん
お疲れ様です。
QuestionのAPIにユーザーIDでの絞り込み機能をつける形に変更いたしました。
お手隙の際に、ご確認のほどよろしくお願いします🙏

@garammasala29 garammasala29 assigned machida and unassigned machida Mar 14, 2022
komagata
komagata reviewed Mar 14, 2022
View reviewed changes
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.

同じコミットログが続いているところがあるので、メッセージやコミット粒度を見直してみると良いかもです〜

@garammasala29 garammasala29 force-pushed the feature/convert-users-questions-list-to-vuejs branch from 3096ba3 to 387b1ef Compare March 14, 2022 22:16
@garammasala29
Copy link
Contributor Author

@komagata さん
お疲れ様です。コミットログの重複修正しましたので、ご確認お願いします🙏

komagata
komagata reviewed Mar 15, 2022
View reviewed changes
test/integration/api/questions_test.rb Outdated
@@ -43,4 +43,15 @@ class API::QuestionsTest < ActionDispatch::IntegrationTest
assert_equal changed_title, @question.reload.title
end
end

test 'get users question with REST API' do
Copy link
Member

Choose a reason for hiding this comment

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

このアプリのコンテクストではAPIといえば全部REST APIなので単にAPIと書いちゃっていいかもです。

Graphqlとか別のスタイルのAPIを併用しているサイトであれば書いた方がいいかもという感じです〜。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Graphqlの存在は存じ上げなかったので、勉強になりました。
810e946で修正しました。

@garammasala29
Copy link
Contributor Author

@komagata さん
再度修正致しました。ご確認よろしくお願いします。

komagata
komagata reviewed Mar 15, 2022
View reviewed changes
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.

このPRでの変更点とは別になりますが、このテストファイルの他のテスト名も変更お願いできるとありがたいです。

https://github.com/fjordllc/bootcamp/blob/810e9468623d5b76f996362831514aae62b2fedd/test/integration/api/reports_test.rb

他のAPIのテストを参考にこちらのような感じにしていただければ〜。

Paru871
Paru871 reviewed Mar 15, 2022
View reviewed changes
db/fixtures/questions.yml Outdated Show resolved Hide resolved
@garammasala29 garammasala29 force-pushed the feature/convert-users-questions-list-to-vuejs branch from 6f8200c to 3ab97fb Compare March 16, 2022 00:29
@garammasala29
Copy link
Contributor Author

@komagata さん
お疲れ様です。貼付いただいたURLを参考にテスト名を変更しました。ご確認よろしくお願いします！

@eatplaynap eatplaynap mentioned this pull request Mar 18, 2022
Merged
komagata
komagata approved these changes Mar 18, 2022
View reviewed changes
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 e53d82d into main Mar 18, 2022
@komagata komagata deleted the feature/convert-users-questions-list-to-vuejs branch March 18, 2022 17:59
@github-actions github-actions bot mentioned this pull request Mar 18, 2022
Merged
17 tasks
@garammasala29 garammasala29 mentioned this pull request Mar 24, 2022
Closed
@clio209 clio209 mentioned this pull request Apr 2, 2022
Merged
@taka61 taka61 mentioned this pull request Apr 4, 2022
Merged
@tomonariha tomonariha mentioned this pull request Jul 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Paru871 Paru871 Paru871 left review comments

@aim2bpg aim2bpg aim2bpg approved these changes

@komagata komagata komagata approved these changes

Assignees

@garammasala29 garammasala29

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@garammasala29 @aim2bpg @komagata @Paru871 @machida