全てのタグを全てのタブで表示するため、コントローラーから全てのタグを渡すように変更 #5488
Conversation
|
@pachikuriii 前回とはまた少し違った箇所のレビューを依頼させていただきました(__) 自分の方もできることがあれば気にせずにお声がけくださいませ。 |
|
@yuki-snow1823 こちらのレビュー、明日取り掛かりますので少しお待ちいただけますと幸いです🌱 |
| @@ -20,7 +20,7 @@ def index | |||
| Question.all | |||
| end | |||
| @tag = ActsAsTaggableOn::Tag.find_by(name: params[:tag]) | |||
| @tags = questions.all_tags | |||
| @tags = Question.all.all_tags | |||
There was a problem hiding this comment.
🙏ご参考まで🙏
ビュー側でもQuestion.all.all_tagsが参照されてコントローラーと重複しているので
コントローラーで定義した@tagsをview側でも使ってあげるのはいかがでしょうか✨
👇app/views/questions/index.html.slim
-
現在
-
ご提案
There was a problem hiding this comment.
@pachikuriii
お疲れ様です。
スマートに変更されてますね〜✨🙌！わたしの環境でも動作確認ばっちりできました。
こういった一言、非常に嬉しかったです。色々考えてから一番最小限に変更可能で、影響の少なさを考えた結果ここだけ変えれば良さそうかなと思った次第です。自分もこういう一言心がけようと思いました！
何かの意図があってslimに直接Question.allのように記載した可能性もありますが、自分が見た限りは動作確認して問題なさそうだったので、再レビューお願いします。
こちらのコミットで修正しました。
#5488
There was a problem hiding this comment.
ご提案の採用ありがとうございます〜💪🏻🌈
自分もこういう一言心がけようと思いました！
先輩方とメンターさんのレビューをまねっこしてみました〜！👻笑
何かの意図があってslimに直接Question.allのように記載した可能性もありますが
当初は@tags = questions.all_tagsとなっていたので@tagsには選択したタブで表示される質問のタグだけが定義されていました。なのでslimファイルでQuestion.all.all_tagsとすることで選択されたタブに関わらず、どのタブでも全ての質問のタグを表示させる仕様を実現していたんでしょうね〜...と意図を勝手に推測してみました。
わたしからはApproveとさせていただきますね〜🙌
|
@pachikuriii @komagata |
console.log($(".page-tags-nav__item").length)本番環境で全てのタブで数が変わらないことを確認し、表示にも問題ないので大丈夫です。完了に移します。 |
Issue
概要
Q&Aのページには3つのタブが存在している。「未解決」「解決済み」「全て」である。
例えば、未解決の質問についているタグの一覧は解決済みの方には表示されない。
一方で、タグ一覧は質問の種類に関係なく全てのタグの一覧として表示されているのが望ましい。
本プルリクエストでは、それを解決した。
詳細
view側は変更せずに、コントローラーから渡す
@tagsの値をタブごとではなくQuestion全てに紐づくものにしました。これによって、タグが存在していない場合はli要素の作成がされず、質問に関連するタグが存在している場合のみHTML要素が描画されると考えました。
変更確認方法
show-all-tags-in-q-and-aをローカルに取り込むbin/rails sでローカル環境を立ち上げる※タグの保存を押下する直前の画面
こちらだとダメです。一度EnterかReturnキーを押下してください。
変更前後で見た目上の変化はありません（見た目は変わらず、出力されるものが変わります）
その他
ブランチ名を間違えたのでcloseして作り直しました。