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

Q&A個別ページに同プラクティス関連Q&A一覧を追加する #6135

Merged
merged 7 commits into from
Feb 18, 2023

Conversation

futa4095
Copy link
Contributor

@futa4095 futa4095 commented Jan 30, 2023

Issue

概要

Q&A個別ページに表示している質問と同じプラクティスの質問一覧を追加しました。

  • 質問一覧には最大20件表示します
  • 質問一覧にWIPの質問は含みません
  • 一覧部分にはタイトル、公開日時、回答数、未解決かを表示します
  • 「全て見る」でQ&A一覧ページにリンクします

変更確認方法

  1. feature/add-category-listing-to-question-pageをローカルに取り込む
  2. rails sでローカル環境を立ち上げる
  3. http://localhost:3000/questionsにアクセス
  4. 質問タイトル(テストの質問40など)をクリックしQ&A個別ページを表示
  5. プラクティスに紐づく質問の一覧を表示を確認する

Screenshot

変更前

image

変更後

image

@futa4095
Copy link
Contributor Author

@machida こちらのPR、見た目以外の機能ができたのでデザインの追加をお願いします!!

@machida
Copy link
Member

machida commented Feb 1, 2023

@futa4095 デザイン了解ですー

@machida machida force-pushed the feature/add-category-listing-to-question-page branch from 6cbbf04 to 3555bc9 Compare February 1, 2023 13:56
@machida
Copy link
Member

machida commented Feb 1, 2023

@futa4095 デザイン入れました!最新の main を取り込んだので手元で、

git pull --rebase origin feature/add-category-listing-to-question-page

をお願いしますー

@futa4095 futa4095 marked this pull request as ready for review February 3, 2023 06:36
@futa4095 futa4095 requested a review from peno022 February 3, 2023 06:37
@futa4095
Copy link
Contributor Author

futa4095 commented Feb 3, 2023

@peno022 レビューお願いしたいです!!
ご都合いかがでしょうか?

Copy link
Contributor

@peno022 peno022 left a comment

Choose a reason for hiding this comment

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

レビュー依頼ありがとうございます!
動作は問題なかったです🙋‍♀️

2点コメントしましたので、確認お願いします〜!

footer.page-nav__footer
= link_to questions_path(practice_id: @question.practice),
class: 'page-nav__footer-link' do
| 全て見る
Copy link
Contributor

@peno022 peno022 Feb 3, 2023

Choose a reason for hiding this comment

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

issueの町田さんとの確認内容の中で記載されていた「もっと見る」の文言から、「全て見る」の文言に変更した理由はありますか?

一般的には「もっと見る」の文言のほうが見る機会が多いかな?と思ったのと、遷移先ページでページネーションがあり必ず全件見られるわけではないので、気になりました。

(必ず修正すべきという意味ではなく、意図があればまずお聞きしたいなという感じです! もし特に意図がなければ「もっと見る」でいいのかなとは思いました。)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

町田さんにデザインを入れてもらった中で変更されてました。
Docsが「全て見る」になっていて、Q&Aもそれと合わせたのだと思っています。

Comment on lines 35 to 49
- @practice_questions.each do |question|
li.page-nav__item(class="#{@question == question ? 'is-current' : ''}")
= link_to question_path(question), class: 'page-nav__item-link has-metas' do
.page-nav__item-link-inner
.page-nav__item-header
- unless question.correct_answer
.a-badge.is-danger.is-xs
| 未解決
.page-nav__item-title
= question.title
.page-nav-metas
.page-nav-metas__item
= "公開:#{l question.published_at}"
.page-nav-metas__item
= "回答・コメント(#{question.answers.count})"
Copy link
Contributor

Choose a reason for hiding this comment

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

issueの会話にて、表示項目は

ユーザー名
作成日時
解決済みであるか
回答コメント数

とのことなので、ユーザー名の表示追加が必要かと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

デザイン依頼する前はあったんですが、戻ってきたらなくなっていました。
不要だったので消したのかと思いましたが、問い合わせた方が良いのでしょうか。

Copy link
Contributor

@peno022 peno022 left a comment

Choose a reason for hiding this comment

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

@futa4095
ご回答ありがとうございます!
町田さんのコミットのdiffだけ別途確認というふうにしなかったので、色々背景分からず質問しておりました🙏

意図したものであれば問題ないと思いますので、Approveします!

@futa4095
Copy link
Contributor Author

futa4095 commented Feb 4, 2023

@peno022 Approveありがとうございます!!!!

色々背景分からず質問しておりました

背景がわかるようになっておらず申し訳ありませんでした!
issueと内容が違うので質問されて当然だと思います。
issueに背景・経緯等残すべきだと思いました。

@futa4095 futa4095 requested a review from komagata February 4, 2023 11:05
@futa4095
Copy link
Contributor Author

futa4095 commented Feb 4, 2023

@komagata Approveいただきましたのでレビューお願いします!!


ul.page-nav__items
- @practice_questions.each do |question|
li.page-nav__item(class="#{@question == question ? 'is-current' : ''}")
Copy link
Member

Choose a reason for hiding this comment

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

この辺り、他でも使いそうなのでpartialにしておくと良いかもです〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata
すいません、確認させてください
ulタグのリスト部分を丸ごとpartialにするのでしょうか?
それともliタグの行の部分をpartialにするべきでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

@futa4095 ul(Q&A全体)がいいかな〜と思います

@machida machida removed their assignment Feb 15, 2023
@futa4095 futa4095 force-pushed the feature/add-category-listing-to-question-page branch from 8d6da63 to 9a84752 Compare February 15, 2023 11:30
@futa4095
Copy link
Contributor Author

@komagata 一覧部分をpartialにしました!再度レビューお願いします!

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 5425e47 into main Feb 18, 2023
@komagata komagata deleted the feature/add-category-listing-to-question-page branch February 18, 2023 02:18
@github-actions github-actions bot mentioned this pull request Feb 18, 2023
10 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.

4 participants