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

ブログ記事の一覧にて、feature タグがついてるブログ記事はメンター・管理者にだけ目印が表示される。 #8186

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ayu-0505
Copy link
Contributor

@ayu-0505 ayu-0505 commented Nov 12, 2024

Issue

概要

ブログ記事の一覧にて、feature タグがついてるブログ記事は、メンター・管理者にのみ目印であるfeatureという文言が表示されるように変更しました。

変更確認方法

  1. feature/highlight-blogs-with-a-feature-tagをローカルに取り込む
    1. git fetch origin pull/8186/head:feature/highlight-blogs-with-a-feature-tag
      (2度目以降は上記ブランチのローカル変更に気をつけながら--forceをつけてください)
    2. git switch feature/highlight-blogs-with-a-feature-tag
  2. foreman start -f Procfile.devでローカルサーバーを立ち上げる
  3. メンターまたは管理者(例: user:komagata, password: testtest)でログインする。
  4. ブログの新規作成ページにアクセスし、タグ入力欄でfeatureを入力し、エンターを押してタグを登録する。
  5. タイトル、本文、サムネイル画像を適当に入力し、公開するボタンをクリックする。
  6. ブログ記事の一覧にアクセスし、先ほど作成したブログ記事に目印である注目の記事の文字が表示されているか確認する。
  7. 一旦ログアウトし、ブログ記事の一覧に再度アクセス。
  8. 先ほど作成したブログ記事に目印である注目の記事の文字が表示されていないか確認する。
  9. メンターまたは管理者以外で再度ログインする(例: user:kimura, password: testtest
  10. ブログ記事の一覧にアクセスし、先ほど作成したブログ記事に目印である注目の記事の文字が表示されていないか確認する。

Screenshot

変更前

tag-testのブログにはfeatureタグをつけており、メンターまたは管理者としてログインしているが、目印は表示されない
スクリーンショット 2024-11-12 12 59 23

変更後

メンターまたは管理者としてログイン時

スクリーンショット 2024-11-19 11 58 36

非ログイン時

スクリーンショット 2024-11-19 11 59 35

管理者外でログイン中

スクリーンショット 2024-11-19 12 09 47

@ayu-0505 ayu-0505 self-assigned this Nov 12, 2024
@ayu-0505
Copy link
Contributor Author

@machida さん、お疲れ様です🍵

こちらのPRですが、デザインが必要だと思われますがいかがでしょうか。
現在はfeatureという素の文字列がPタグで表示されている状態です。
また、CSSクラスについては周囲の状況から考え、.thumbnail-card__metaのみ記載しております。

ご確認のほどよろしくお願いいたします🙇🏻‍♀️

@machida
Copy link
Member

machida commented Nov 12, 2024

@ayu-0505 デザイン入れてこのブランチにPR出しますー

@machida
Copy link
Member

machida commented Nov 13, 2024

@ayu-0505 ちょっとした変更だったのでこのブランチにpushしましたー。ご確認お願いします🙏 feature 以外にも卒業生インタビューなど特別なタグを用意する予定で、特別なタグを複数持つ記事も出るかもしれないので、それに対応できるようなHTMLとCSSにしました。

@ayu-0505
Copy link
Contributor Author

@machida さん、対応いただきありがとうございました🙏

@ayu-0505 ayu-0505 marked this pull request as ready for review November 19, 2024 04:19
@ayu-0505
Copy link
Contributor Author

@hagiya0121 さん、お疲れ様です🍵

もしよろしければこちらのPRのレビューをお願いできますでしょうか。
お忙しい場合は遠慮なくご連絡いただけたらと思います。
ご確認のほどよろしくお願いします🙇🏻‍♀️

@hagiya0121
Copy link
Contributor

@ayu-0505
了解です!三日以内には終わると思います🙂

Copy link
Contributor

@hagiya0121 hagiya0121 left a comment

Choose a reason for hiding this comment

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

@ayu-0505
動作に問題はありませんでした。
気になったところをコメントしたので確認お願いします🙂

@@ -7,7 +7,7 @@ class ArticlesController < ApplicationController

def index
@articles = Article.with_attachments_and_user.order(created_at: :desc).page(params[:page])
Copy link
Contributor

Choose a reason for hiding this comment

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

ここでpreloadしていないのでparams[:tags]が空のときにN+1問題が発生していました。

Copy link
Contributor Author

@ayu-0505 ayu-0505 Nov 22, 2024

Choose a reason for hiding this comment

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

ありがとうございます!
ログが正確に見れておらずN+1が発生していることに気づきませんでした💦
こちら修正いたしました。

Copy link
Contributor

Choose a reason for hiding this comment

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

@ayu-0505
すいません修正したコミットが見当たらないです🤔

Copy link
Contributor Author

@ayu-0505 ayu-0505 Nov 22, 2024

Choose a reason for hiding this comment

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

@hagiya0121
すみません、こちらmainとのリベースでコンフリクトを起こしていまして、コンフリクトの修正に慣れていないがために初回のN+1の修正コミットに吸収されていました。
(コンフリクトの修正がコミットごとである認識が不足したためにこうなってしまいました)
N+1を修正
こちらをみていただけたらと思います🙏

@@ -35,6 +35,12 @@ hr.a-border-tint
.col-lg-4.col-md-6.col-xs-12
.thumbnail-card.a-card class=(article.wip? ? ' is-wip' : '')
= link_to article, class: 'thumbnail-card__inner' do
- if current_user&.admin_or_mentor_login? && article.tags.pluck(:name).include?('feature')
Copy link
Contributor

Choose a reason for hiding this comment

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

どちらでもいいと思うのですがarticle.tags.pluck(:name).include?('feature')はロジックなのでviewに書かずに/app/helpers/articles_helper.rbに書いてもいいかなと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに、と思いましたので、こちらも/app/helpers/articles_helper.rbに移動させました。

@ayu-0505 ayu-0505 force-pushed the feature/highlight-blogs-with-a-feature-tag branch from f9bd799 to ef4a1f2 Compare November 21, 2024 07:56
@ayu-0505
Copy link
Contributor Author

@hagiya0121 さん、確認いただきありがとうございました🍵
修正いたしましたので、またお時間ある時に確認いただけたらと思います🙏

Copy link
Contributor

@hagiya0121 hagiya0121 left a comment

Choose a reason for hiding this comment

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

@ayu-0505
修正ありがとうございます。確認OKです🙏
Approveします。

@ayu-0505
Copy link
Contributor Author

@komagata さん、お疲れ様です🍵

メンバーレビューでApproveいただいたので、こちらお手隙の際に確認をお願いいたします🙏

@@ -18,4 +18,8 @@ def meta_robots_tag
content = logged_in? ? 'none' : 'noindex, nofollow'
tag.meta(name: 'robots', content:)
end

def feature_tag?(article)
Copy link
Member

Choose a reason for hiding this comment

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

helperに独自のメソッドを追加した場合は対応するテストが1つはほしい感じです〜

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