コース毎の参考書籍一覧ページを非Vue化した #8158

ai-24 · 2024-10-27T13:34:31Z

Issue

/courses/:course_id/books を非Vue化したい #8052

概要

Vueで実装されていた/courses/:course_id/books をRailsでの実装に書き換えました。

変更確認方法

chore/change-courses-id-books-page-from-vue-to-htmlをローカルに取り込む
bin/setupを実行
http://localhost:3000/coursesにkomagataさんでアクセスし、Railsエンジニアコースをクリック
Railsエンジニアコースのプラクティスが表示されるので、上部タブの書籍をクリック

Railsエンジニアコースの参考書籍ページが、Vueで実装していた時と変わらず表示されているか確認する
書籍ページ内の必読タブをクリックし、必読書籍も同じようにVueで実装していた時と変わらず表示されているか確認する

スクール生(kimuraやhajimeなど)でログインし5 6のページ内で参考書籍の編集ボタンが表示されないことを確認する

Screenshot

画面上の変更はありません。

ai-24 · 2024-10-28T02:53:47Z

@ham-cap
お疲れ様です！
お忙しいところ申し訳ありませんが、お手隙の際にこちらのレビューをお願いできますでしょうか？
ご多忙かと思いますので、難しければ遠慮なくおっしゃってください！
よろしくお願いいたします🙇‍♀️

ham-cap · 2024-10-29T00:39:03Z

@ai-24
お疲れさまです！
レビュー依頼ありがとうございます！
今週は少しバタバタするのですが、レビューは是非やらせていただきたいので1週間ほどお時間いただければ幸いです🙏
もしお急ぎの場合はお知らせください！

ai-24 · 2024-10-29T21:58:40Z

@ham-cap
ありがとうございます！
急ぎませんのでよろしくお願いいたします🙇‍♀️

ham-cap

@ai-24
大変お待たせしました🙇‍♂️
いくつか気になった点をコメントさせていただきましたのでご確認いただければと思います〜🙏

ham-cap · 2024-11-05T01:16:09Z

app/views/courses/books/index.html.slim

+                              li.tag-links__item
+                                = link_to practice.title, practice_path(practice), class: 'tag-links__item-link'
+                      hr.a-border-tint
+                      - if current_user.admin? || current_user.mentor?


管理者かメンターでログインしていることを確認するメソッドがconcernsにあるので、そちらを使用するほうがいいかもしれません👀
以下、当該メソッド定義場所のリンクです。
https://github.com/fjordllc/bootcamp/blob/main/app/controllers/concerns/authentication/login_helpers.rb#L26

ありがとうございます！
そのようなメソッドがあることを把握できておりませんでした！
ご提案いただいたメソッドを見ていて、このページ自体がログインしていないと表示されないものなので、admin_or_mentor?が最適かと考えました。
いかがでしょうか？🤔

admin_or_mentor?で大丈夫だと思います！🙆‍♂️

ham-cap · 2024-11-06T00:51:01Z

app/views/courses/books/index.html.slim

+                                      | 必読
+                                  = link_to book.page_url, class: 'card-books-item__title-link', target: '_blank', rel: 'noopener'
+                                    span.card-books-item__title-label
+                                      = book.title


Slimを使用しているので、テキストはなるべく|を使用してあげる方がいいかもしれません👀

span.card-books-item__title-label | #{book.title}

slimでrubyコードを出力するときは=を使うことができるので、book.titleを#{ }で囲って表示させるのではなく、= book.titleとした方がシンプルな気がしました🤔
slimのREADMEも見に行き問題ないと判断しましたが、認識違いしている部分などありましたらお知らせいただけると幸いです🙏

https://github.com/slim-template/slim/blob/main/README.jp.md

たしかに感覚的に= book.titleのほうがシンプルというのは分かる気がします👀
ただ個人的にBootcamp内では|を使用している場合が多い印象があるのと、私自身もレビューで同じ指摘を受けたことがあるので今回お伝えさせていただいた次第です🤔
とはいえ、自分もそのあたりのルールがどこかに明記されているのを見たことがなく、明確にどちらがいいと言える根拠を持っていないので、これについてはお任せします🙏
指摘しておいて中途半端な回答になってしまってすみません💧

なるほど！
ご回答ありがとうございます！
よく|を使って書かれている＋レビューでも指摘があったとのことでしたらご提案していただいた書き方の方が良いと判断いたしました。
こちらも変更しております🙇‍♀️

ham-cap · 2024-11-06T00:53:45Z

app/views/courses/books/index.html.slim

+                                      = book.title
+                              .card-books-item__row
+                                p.card-books-item__price
+                                  = "#{book.price.to_s(:delimited)}円（税込）"


こちらも上のbook.titleと同様に|のほうがいいような気がします〜

| #{book.price.to_s(:delimited)}円（税込）

おっしゃる通り|を使って書いた方が分かりやすいですね！
変更しておりますのでご確認よろしくお願いいたします🙇‍♀️

ham-cap · 2024-11-06T01:05:12Z

app/views/courses/books/index.html.slim

+                                  .card-books-item__description
+                                    .a-short-text
+                                      p
+                                        = safe_join(book.description.split(/\n/), tag.br)


pを使用していてクラス名も特にないので、改行せずにそのまま文字列として渡してしまってもいいように思いますが、好みの問題かもしれないので判断はお任せします🙌

p #{safe_join(book.description.split(/\n/), tag.br)}

こちら私も改行なしで書こうか迷ったのですが、今後クラスやIDをつけたくなった時に、改行してある方がdiffが減ると考えたのと、他にもpだけで書かれているところを複数見かけたので改行した経緯がありました。（クラスやIDをつける可能性は低いかもしれませんが、、、）
なので特にチームでこの場合は改行しないなどのルールがなく、特に問題なければこのままにしておこうかと思います🤔

simple_formatなどが使えるかも？です〜

ham-cap · 2024-11-06T02:10:18Z

app/controllers/courses/books_controller.rb

@@ -3,5 +3,6 @@
 class Courses::BooksController < ApplicationController
  def index
    @course = Course.find(params[:course_id])
+    @books = Book.filtered_books(params[:status], @course)


BookやCourseに定義されたメソッドも含めて、@booksを取得するロジックが少し複雑になってしまっているようなので、モデル内のメソッドではなくActiveRecordの関連付けを使ってコントローラー内で完結したほうがいいような気がします👀
例えばなのですが、以下のような感じでparams[:status]の値を先に判定してしまい、コースに紐づく書籍の取得はテーブルを連結して一気にやってしまうというのはいかがでしょうか？🤔

def index @course = Course.find(params[:course_id]) must_read_status = params[:status] == 'mustread' ? { practices_books: { must_read: true } } : {} @books = Book.joins(practices: { categories: :courses_categories }, practices_books: :practice) .where(courses_categories: { course_id: @course.id }) .where(must_read_status) .distinct .order(updated_at: :desc, id: :desc) end

これであれば、BookとCourseのモデルをいじらなくてもコースに紐づいた書籍の取得はできそうです。
今回の場合BookからCourseまでを繋げる関連付けが複雑なのと、自分もActiveRecordの扱いが上手いわけではないのであまり自信はないのですが、コードの見通しは改善できると思うので、一つの提案としてご検討いただければ幸いです🙏
もし仕様やモデル間の関連付けについて私の理解に誤りがあれば遠慮なくご指摘ください〜🙌

ありがとうございます！
おっしゃる通りご提案していただいたように書いた方が分かりやすいと思いました🙇‍♀️
この辺りあまり得意ではなかったので、コードも考えてくださって非常に勉強になりました！
いただいたコードに変更を少し加えております。
変更点はN+1を考慮したのとmust_read_statusをメソッドにしました。
ご確認いただければと思います！

ai-24 · 2024-11-19T13:03:06Z

@ham-cap さん
お忙しい中レビューありがとうございました🙇‍♀️
大変勉強になりました。
お時間いただいてしまいましたが修正とコメントしております。
何か認識間違いしている部分などありましたら遠慮なくお知らせいただければと思います！
急ぎませんのでご確認よろしくお願いいたします🙏

ham-cap

@ai-24
ご対応ありがとうございます😃
一応それぞれのコメントに返信させていただいていますが、特に問題ないと思いますのでApproveさせていただきます〜🙆‍♂️

ai-24 · 2024-11-24T10:59:21Z

@ham-cap
ありがとうございます🙏
= book.titileは| #{book.title}に変更した方が良いと判断したので修正いたしました！
お忙しい中レビューして頂きありがとうございました🙇‍♀️

@komagata
お疲れ様です！
チームメンバーのレビューが通りましたので、こちらのレビューをお願いいたします🙇‍♀️

ai-24 self-assigned this Oct 27, 2024

ai-24 requested a review from ham-cap October 29, 2024 22:01

ham-cap reviewed Nov 6, 2024

View reviewed changes

ai-24 force-pushed the chore/change-courses-id-books-page-from-vue-to-html branch from 1c29ddd to d223b75 Compare November 17, 2024 13:11

ham-cap approved these changes Nov 20, 2024

View reviewed changes

ai-24 added 5 commits November 23, 2024 08:27

/courses/:course_id/booksを非Vue化した

3d51cc9

/courses/:course_id/booksを非Vue化したためファイルを削除した

a09a5a2

データの取得方法を変更

4adb20a

コードの書き方をシンプルにした

63ea816

コードの書き方を変更

f16ee13

ai-24 force-pushed the chore/change-courses-id-books-page-from-vue-to-html branch from d223b75 to f16ee13 Compare November 22, 2024 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

コース毎の参考書籍一覧ページを非Vue化した #8158

コース毎の参考書籍一覧ページを非Vue化した #8158

ai-24 commented Oct 27, 2024 •

edited

Loading

ai-24 commented Oct 28, 2024

ham-cap commented Oct 29, 2024

ai-24 commented Oct 29, 2024

ham-cap left a comment

ham-cap Nov 5, 2024

ai-24 Nov 18, 2024 •

edited

Loading

ham-cap Nov 20, 2024

ham-cap Nov 6, 2024

ai-24 Nov 18, 2024

ham-cap Nov 20, 2024

ai-24 Nov 22, 2024

ham-cap Nov 6, 2024

ai-24 Nov 18, 2024

ham-cap Nov 6, 2024

ai-24 Nov 19, 2024 •

edited

Loading

komagata Nov 28, 2024

ham-cap Nov 6, 2024

ai-24 Nov 19, 2024

ai-24 commented Nov 19, 2024

ham-cap left a comment

ai-24 commented Nov 24, 2024

コース毎の参考書籍一覧ページを非Vue化した #8158

Are you sure you want to change the base?

コース毎の参考書籍一覧ページを非Vue化した #8158

Conversation

ai-24 commented Oct 27, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

ai-24 commented Oct 28, 2024

ham-cap commented Oct 29, 2024

ai-24 commented Oct 29, 2024

ham-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai-24 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai-24 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai-24 commented Nov 19, 2024

ham-cap left a comment

Choose a reason for hiding this comment

ai-24 commented Nov 24, 2024

ai-24 commented Oct 27, 2024 •

edited

Loading

ai-24 Nov 18, 2024 •

edited

Loading

ai-24 Nov 19, 2024 •

edited

Loading