-
Notifications
You must be signed in to change notification settings - Fork 72
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
/sponsorshipsページの新規作成 #8338
base: main
Are you sure you want to change the base?
/sponsorshipsページの新規作成 #8338
Conversation
@harada-webdev |
@SuzukiShuntarou また、ログイン後の/sponsorshipsにバグがあることが確認できました。(WIPボタンを押すと、/articles/wipsに遷移してしまう) Clipchamp.mp4以上、問題が解決されましたらレビューをいたしますので、お手数ですが再度ご確認のほどお願いいたします! |
@harada-webdev |
05602f2
to
a6ac8b4
Compare
@harada-webdev
|
@SuzukiShuntarou |
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.
@SuzukiShuntarou
二点ほど修正したほうがいい箇所があったのでコメントしました!
ご確認のほどお願いいたします🙏
|
||
class Articles::SponsorshipsController < ApplicationController | ||
skip_before_action :require_active_user_login, raise: false, only: %i[index] | ||
MAX_PAGE_COUNT = 12 |
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.
他のController
で定義されている1ページあたりの要素数を格納している定数名のほとんどがPAGER_NUMBER
なので、その定数名を踏襲した方が良いのではないかと考えました。(grep -r "PAGER_NUMBER" app/controllers
で確認できます)
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.
ありがとうございます!他コントローラーでも使っているのを確認いたしました。PAGER_NUMBER
に変更しました。
|
||
def sponsorships_articles | ||
Article.with_attached_thumbnail.includes(user: { avatar_attachment: :blob }) | ||
.where(thumbnail_type: :sponsorship, wip: false).order(created_at: :desc).page(params[:page]) |
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.
ブログ記事を並べる順番について、
ArticlesController
で記事を並べるためのメソッドsorted_articlesがpublished_at
の順で並べているのと、ブログ記事を編集する際に記事公開日時を任意で変更できるのですが、その変更された新しい日時情報を格納しているカラムがpublished_at
なので、created_at
の順よりもpublished_at
の順でブログ記事を並べたほうがよいのではないかと思いました。
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.
ありがとうございます!こちらも動作確認、該当のメソッドも確認いたしました。
ご指摘の通りで created_at
で並び替えを行うと/articles
と異なる並び順となっておりました。
published_at
で並び替えを行うように変更いたしました。
@SuzukiShuntarou |
@harada-webdev |
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.
@SuzukiShuntarou
修正を確認できましたので、Approveにいたしました😊
@harada-webdev @komagata |
2c35559
to
214dafb
Compare
Issue
概要
/sponsorships
ページを新規に追加し、sponsorship
タグの付いたブログ記事を1ページ当たり最新12個表示。ログインしていない状態でも該当のページを閲覧可能。
変更確認方法
feature/create-sponsorships-page
をローカルに取り込む。git fetch origin feature/create-sponsorships-page
git checkout feature/create-sponsorships-page
rails db:seed
を実行してsponsorship
タグのブログ記事の初期データを追加する。foreman start -f Procfile.devでローカル環境を立ち上げる。
未ログイン状態でヘッダー部のブログをクリックする。
未ログイン状態で
/sponsorships
ページへアクセスするSponsorships
サムネイル画像の記事のみが表示されることを確認する。ユーザー名
komagata
パスワードtesttest
でログインする。ログイン状態で
/sponsorships
ページへアクセスするSponsorships
サムネイル画像の記事のみが表示されることを確認する。Screenshot
変更前
/sponsorships
ページへのアクセスフッター

変更後
/sponsorships
ページへのアクセスフッター
