-
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
プラクティスのOGP画像を1200px × 630pxにリサイズして表示する #7775
base: main
Are you sure you want to change the base?
Conversation
@sochi419 |
承知しました! |
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.
遅くなり申し訳ありません。
動作確認はOKでした!
プラクティスに画像を添付すると1200px × 630pxにリサイズされることを確認しました。
また、Xとfacebookで、リサイズされた状態で画像が表示されたことを確認しました。
コードに関して、1点だけ細かいですがコメントしていますので確認お願いします🙏
app/decorators/practice_decorator.rb
Outdated
OGP_IMAGE_SIZE = [1200, 630].freeze | ||
|
||
def ogp_image_url | ||
ogp_image.variant(resize_to_fill: OGP_IMAGE_SIZE).processed.url if ogp_image.attached? |
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.
if ogp_image.attached?
と定義しているのですが、views側でも同様のif文が記述されています。
こちらのif文に関しては、modelかviewsどちらか一方に記述すればいいと思ったのですがいかがでしょうか?
- app/views/practices/_summary.html.slim(抜粋)
- if practice.ogp_image.attached?
= image_tag practice.ogp_image_url
- app/views/practices/completion/show.html.slim(抜粋)
if @practice.ogp_image.attached?
set_meta_tags(og: { image: @practice.ogp_image_url, url: request.url })
set_meta_tags(twitter: { image: @practice.ogp_image_url, url: request.url })
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.
まず、view側のif文は各viewにおいてその後の処理が異なるので基本的に必須だと考えます。
そこで、Decorator側のメソッドogp_image_url
内でのif文が重複しているという意図だと認識しています。
このogp_image_url
が、今後もしもチェックをせずに使用された場合、このメソッドで以下のようなエラーが発生することになります。
undefined method `processed' for nil:NilClass
ogp_image.variant(resize_to_fill: OGP_IMAGE_SIZE).processed.url
^^^^^^^^^^
なのでogp_image_url
側の条件文も必要だと考えているのですが、いかがでしょうか。
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.
なるほど、エラーが発生してしまうのですね..
であれば、ogp_image_url側の条件文も必要だと私も思います。
(わざわざerror検証して頂きありがとうございました。)
レビューしました! |
@sochi419 |
レビューの返信ありがとうございました! |
@komagata 既存の実装を参考に、プラクティスのogp_imageがattachされていない場合、デフォルト画像を返すように修正しようと思うのですが、ogp_imageのデフォルト画像として指定する画像は以下でよろしいでしょうか?
|
@naokinaokiboo 基本的にはOKですが、プラクティスのOGP以外にもブログのOGPなどいろんな種類があるとおもうので、そのあたりデフォルトの画像がどうあればよいのかについて @machida さんと相談してみてください~。 |
@machida まず経緯としては、対応にあたり、リサイズしたogp画像を返却するメソッドを追加しようとしています。
|
返信遅れてすいません!!
これでお願いしますー🙏 |
9917d24
to
3ede618
Compare
@komagata |
app/decorators/practice_decorator.rb
Outdated
@@ -0,0 +1,17 @@ | |||
# frozen_string_literal: true | |||
|
|||
module PracticeDecorator |
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.
データを変更している(副作用が発生している)場合はDecorationの範疇を出ていると思うので、他と同じようにモデルの方に書いたほうがいいかもです。
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.
確かに、元のファイルからサイズ違いの画像を作成・保存していて、表示に関するロジックだけとはいえないですね。💦
モデルに記載するようにします。
3ede618
to
b3a831f
Compare
b3a831f
to
95d7b58
Compare
@komagata |
def ogp_image_url | ||
default_image_path = '/ogp/ogp.png' | ||
|
||
if ogp_image.attached? | ||
ogp_image.variant(resize_to_fill: OGP_IMAGE_SIZE, autorot: true, saver: { strip: true, quality: 60 }).processed.url | ||
else | ||
image_url default_image_path | ||
end | ||
rescue ActiveStorage::FileNotFoundError, ActiveStorage::InvariableError, Vips::Error | ||
image_url default_image_path | ||
end |
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.
他のモデルでやっているようなこういうメソッドを用意するのは、ActiveStorageのバージョンが古い頃にすごく遅いという制限があったためにやっていることなので、新しく実装するこちらは普通に直接ogpを表示しちゃって大丈夫です。
Issue
概要
プラクティスに設定できるOGP画像について、1200px × 630pxでリサイズして使われるよう変更しました。
画像のリサイズの仕様は以下のIssueに記載されています。
X(Twitter)で投稿した場合のOGP画像の表示
X(Twitter)で表示されるOGP画像に関しては、1200px × 630pxにリサイズされた画像が、さらに正方形に切り抜かれて表示されます。
(Summary Card | Docs | Twitter Developer Platform)
Facebookで投稿した場合のOGP画像の表示
Facebookで表示されるOGP画像に関しては、1200px × 630pxにリサイズされた画像が、さらにアスペクト比 1.91:1 にリサイズされて表示されます。
(Images in Link Shares - Sharing)
変更確認方法
feature/resize-ogp-image-in-practice
をローカルに取り込むOGP画像の選択/変更の項目の説明文の確認
/mentor/practices/new
)、プラクティス編集ページ(/mentor/practices/[practice_id]/edit
)にアクセスし、OGP画像の選択/変更の補足説明文が以下のように表示されていることを確認する。「ここにアップロードした画像が、X(Twitter)、Facebook で投稿した際に表示される画像として使われます。画像サイズが 1200px × 630px でない場合は、アスペクト比を維持したまま1200px × 630pxにリサイズして使われます。」
プラクティスのページで表示される画像の確認
/practices/[practice_id]
)にアクセスするX(Twitter)、Facebook投稿時のOGP画像の確認
※
ngrok
を使用して、ローカル環境を一時的に外部へ公開する必要があります。公開方法は以下を参照してください。ngrok の導入手順・使い方 · fjordllc/bootcamp Wiki · GitHub
※テストで使用する画像は任意ですが、念のためテストで使用した画像を以下に置いておきます。
(1000 x 2000, 2000 x 630の画像は内側の四角形がそれぞれ1000px × 630px、1200px × 630pxになるように引いています。)
/practices/[practice_id]
)をX(Twitter)とFacebookでポストするScreenshot
プラクティス編集ページ
変更前
変更後
プラクティス個別ページ
変更前
変更後
X(Twitter)で投稿時
変更前
変更後
Facebookで投稿時
変更前
変更後