-
Notifications
You must be signed in to change notification settings - Fork 71
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化した #8158
base: main
Are you sure you want to change the base?
コース毎の参考書籍一覧ページを非Vue化した #8158
Conversation
@ham-cap |
@ai-24 |
@ham-cap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ai-24
大変お待たせしました🙇♂️
いくつか気になった点をコメントさせていただきましたのでご確認いただければと思います〜🙏
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
管理者かメンターでログインしていることを確認するメソッドがconcerns
にあるので、そちらを使用するほうがいいかもしれません👀
以下、当該メソッド定義場所のリンクです。
https://github.com/fjordllc/bootcamp/blob/main/app/controllers/concerns/authentication/login_helpers.rb#L26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!
そのようなメソッドがあることを把握できておりませんでした!
ご提案いただいたメソッドを見ていて、このページ自体がログインしていないと表示されないものなので、admin_or_mentor?
が最適かと考えました。
いかがでしょうか?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
admin_or_mentor?
で大丈夫だと思います!🙆♂️
| 必読 | ||
= link_to book.page_url, class: 'card-books-item__title-link', target: '_blank', rel: 'noopener' | ||
span.card-books-item__title-label | ||
= book.title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slimを使用しているので、テキストはなるべく|
を使用してあげる方がいいかもしれません👀
span.card-books-item__title-label
| #{book.title}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slimでrubyコードを出力するときは=
を使うことができるので、book.titleを#{ }
で囲って表示させるのではなく、= book.title
とした方がシンプルな気がしました🤔
slimのREADMEも見に行き問題ないと判断しましたが、認識違いしている部分などありましたらお知らせいただけると幸いです🙏
https://github.com/slim-template/slim/blob/main/README.jp.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たしかに感覚的に= book.title
のほうがシンプルというのは分かる気がします👀
ただ個人的にBootcamp内では|
を使用している場合が多い印象があるのと、私自身もレビューで同じ指摘を受けたことがあるので今回お伝えさせていただいた次第です🤔
とはいえ、自分もそのあたりのルールがどこかに明記されているのを見たことがなく、明確にどちらがいいと言える根拠を持っていないので、これについてはお任せします🙏
指摘しておいて中途半端な回答になってしまってすみません💧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど!
ご回答ありがとうございます!
よく|
を使って書かれている+レビューでも指摘があったとのことでしたらご提案していただいた書き方の方が良いと判断いたしました。
こちらも変更しております🙇♀️
= book.title | ||
.card-books-item__row | ||
p.card-books-item__price | ||
= "#{book.price.to_s(:delimited)}円(税込)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらも上のbook.title
と同様に|
のほうがいいような気がします〜
| #{book.price.to_s(:delimited)}円(税込)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
おっしゃる通り|
を使って書いた方が分かりやすいですね!
変更しておりますのでご確認よろしくお願いいたします🙇♀️
.card-books-item__description | ||
.a-short-text | ||
p | ||
= safe_join(book.description.split(/\n/), tag.br) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p
を使用していてクラス名も特にないので、改行せずにそのまま文字列として渡してしまってもいいように思いますが、好みの問題かもしれないので判断はお任せします🙌
p #{safe_join(book.description.split(/\n/), tag.br)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちら私も改行なしで書こうか迷ったのですが、今後クラスやIDをつけたくなった時に、改行してある方がdiffが減ると考えたのと、他にもp
だけで書かれているところを複数見かけたので改行した経緯がありました。(クラスやIDをつける可能性は低いかもしれませんが、、、)
なので特にチームでこの場合は改行しないなどのルールがなく、特に問題なければこのままにしておこうかと思います🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple_formatなどが使えるかも?です〜
@@ -3,5 +3,6 @@ | |||
class Courses::BooksController < ApplicationController | |||
def index | |||
@course = Course.find(params[:course_id]) | |||
@books = Book.filtered_books(params[:status], @course) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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の扱いが上手いわけではないのであまり自信はないのですが、コードの見通しは改善できると思うので、一つの提案としてご検討いただければ幸いです🙏
もし仕様やモデル間の関連付けについて私の理解に誤りがあれば遠慮なくご指摘ください〜🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!
おっしゃる通りご提案していただいたように書いた方が分かりやすいと思いました🙇♀️
この辺りあまり得意ではなかったので、コードも考えてくださって非常に勉強になりました!
いただいたコードに変更を少し加えております。
変更点はN+1を考慮したのとmust_read_status
をメソッドにしました。
ご確認いただければと思います!
1c29ddd
to
d223b75
Compare
@ham-cap さん |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ai-24
ご対応ありがとうございます😃
一応それぞれのコメントに返信させていただいていますが、特に問題ないと思いますのでApproveさせていただきます〜🙆♂️
d223b75
to
f16ee13
Compare
Issue
概要
Vueで実装されていた
/courses/:course_id/books
をRailsでの実装に書き換えました。変更確認方法
chore/change-courses-id-books-page-from-vue-to-html
をローカルに取り込むbin/setup
を実行http://localhost:3000/courses
にkomagataさん
でアクセスし、Railsエンジニアコースをクリック5
6
のページ内で参考書籍の編集ボタンが表示されないことを確認するScreenshot
画面上の変更はありません。