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

サムネイル画像をOGP画像用のサイズに合わせる #6483

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/controllers/articles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ def create
@article.user = current_user if @article.user.nil?
set_wip
if @article.save
fit_to_size_for_ogp

redirect_to redirect_url(@article), notice: notice_message(@article)
else
render :new
Expand All @@ -41,6 +43,8 @@ def create
def update
set_wip
if @article.update(article_params)
fit_to_size_for_ogp

redirect_to redirect_url(@article), notice: notice_message(@article)
else
render :edit
Expand Down Expand Up @@ -97,4 +101,11 @@ def notice_message(article)
article.wip? ? '記事をWIPとして保存しました' : '記事を更新しました'
end
end

def fit_to_size_for_ogp
return unless @article.thumbnail.attached?

image_resizer = ImageResizer.new(@article.thumbnail)
image_resizer.fit_to!(*ImageResizer::OGP_SIZE)
end
end
3 changes: 1 addition & 2 deletions app/models/article.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ class Article < ApplicationRecord
belongs_to :user
include ActionView::Helpers::AssetUrlHelper

THUMBNAIL_SIZE = '1200x630>'
has_one_attached :thumbnail

before_validation :set_published_at, if: :will_be_published?
Expand All @@ -21,7 +20,7 @@ class Article < ApplicationRecord

def thumbnail_url
if thumbnail.attached?
thumbnail.variant(resize: THUMBNAIL_SIZE).processed.url
thumbnail.blob.url
else
image_url('/images/articles/thumbnails/default.png')
end
Expand Down
37 changes: 37 additions & 0 deletions app/models/image_resizer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# frozen_string_literal: true

class ImageResizer
OGP_SIZE = [1200, 630].freeze

def initialize(attached_one)
@attached_one = attached_one
@transformation = ImageResizerTransformation.new
end

def fit_to!(width, height)
return nil if just_fit_to_size?(width, height)

original_width, original_height = original_size

fitted = @attached_one.variant(**@transformation.fit_to(original_width, original_height, width, height))
.processed
.image.blob

# 変形前の画像関連データの削除にpurge、purge_laterは不要。
# データの削除はattachで担える
@attached_one.attach(fitted)
end

def just_fit_to_size?(width, height)
original_width, original_height = original_size
width == original_width && height == original_height
end

private

def original_size
blob = @attached_one.blob
blob.analyze unless blob.analyzed?
[blob.metadata[:width], blob.metadata[:height]]
end
end
31 changes: 31 additions & 0 deletions app/models/image_resizer_transformation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true

class ImageResizerTransformation
Copy link
Member

@komagata komagata Nov 1, 2023

Choose a reason for hiding this comment

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

実装の都合ではなく、オブジェクト指向でのクラス名・メソッド名の設計を考えた時にちょっと意味がわからない感じがします。

ImageResizerの外部(使う)人が必要なのは、

resizer = ImageResizer.new(attachment)
resizer.resize(width, height)

この部分だけだと思います。

外部からOGP_SIZEというクラス内部のものを外側から注入する必要があるのもおかしいと思います。

実装的に内部を別のクラスにするとしても、現状だと単純にもう一つクラスが必要だから用意した感じで、同じ意味なので、メソッド名も含めて、privateから単純に別のクラスに移動するのではなく、

クラス(名詞)#メソッド(動詞)

という原則を思い出してそれらの組み合わせて設計してみてください。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

外部からOGP_SIZEというクラス内部のものを外側から注入する必要があるのもおかしいと思います。

OGP_SIZEは、ImageResizerクラス自身持つのがおかしいこと理解しました。

リサイズのサイズ決めをするのは、ImageResizerを呼び出すArticleモデルなのでArticleOGP_SIZEを定義する方針で問題ないでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class ImageResizerTransformation

こちらのクラスは、画像をリサイズするのに必要なオプション(variantメソッドの引数)を作成する役割を持つため、以下のように設計してみましたがいかがでしょうか?

  • クラス:ImageResizeOptionBuilder
  • メソッド:build

Copy link
Member

Choose a reason for hiding this comment

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

外部からOGP_SIZEというクラス内部のものを外側から注入する必要があるのもおかしいと思います。

OGP_SIZEは、ImageResizerクラス自身持つのがおかしいこと理解しました。

リサイズのサイズ決めをするのは、ImageResizerを呼び出すArticleモデルなのでArticleOGP_SIZEを定義する方針で問題ないでしょうか?

@wata00913 仕様的にどうなってるのかによりそうです。
例えば「サイト全体でOGP画像のサイズはこうしよう」と決めているのであればサイト全体の設定ファイル的なところにサイズがあるのが適切だと思います。

そうではなく、「Article(ブログ記事)のOGP画像のサイズはこうしよう」と決めていて他とサイズが違うという感じであればArticleに定数を持つのがいいと思います。

Copy link
Member

Choose a reason for hiding this comment

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

class ImageResizerTransformation

こちらのクラスは、画像をリサイズするのに必要なオプション(variantメソッドの引数)を作成する役割を持つため、以下のように設計してみましたがいかがでしょうか?

  • クラス:ImageResizeOptionBuilder
  • メソッド:build

@wata00913 いい感じだと思います。

しかし、ImageResizerとImageRezsizeでほとんど重複しているのでクラス名のプリフィックスで表現するよりImageResizer::OptionBuilderの方がいいと思います。

def fit_to(original_width, original_height, width, height)
# ハッシュを返すが処理順(リサイズ->切り抜き)にオプションを記述
{}.merge(resize(original_width, original_height, width, height),
center_crop(width, height))
end

# 縦横比を維持しながら、リサイズ後の縦をx、横をyとしたときに
# いずれかの条件を満たすようにリサイズする
# x = widht かつ y >= height
# x >= width かつ y = height
def resize(original_width, original_height, width, height)
ratio_w = width.to_f / original_width
ratio_h = height.to_f / original_height

if ratio_w > ratio_h
{ resize: "#{width}x" }
else
{ resize: "x#{height}" }
end
end

def center_crop(width, height)
{ gravity: :center,
# 仮想キャンバスのメタデータをリセットするために!を末尾につける
# +repageは指定できないため、+repage相当の動作をする!を採用
crop: "#{width}x#{height}+0+0!" }
end
end
2 changes: 1 addition & 1 deletion app/views/articles/_form.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
.a-form-help
p
| ここにアップロードした画像が、X(Twitter)、Facebook で投稿した際に表示される画像として使われます。
| 画像サイズは必ず 1200px × 630xpx でお願いします
| 画像サイズが 1200px x 630px でない場合は、登録時に自動で 1200px x 630px に加工されます

- if params[:action] == 'edit'
.form-item
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/active_storage/attachments.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,8 @@ kimuramitai_profile_image:
name: profile_image
record: kimuramitai (User)
blob: kimuramitai_profile_image_blob

article4_thumbnail_image:
name: thumbnail
record: article4 (Article)
blob: article4_thumbnail_blob
3 changes: 3 additions & 0 deletions test/fixtures/active_storage/blobs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,6 @@ komagata_profile_image_blob: <%= ActiveStorage::Blob.fixture filename: "users/av
machida_profile_image_blob: <%= ActiveStorage::Blob.fixture filename: "users/avatars/machida.jpg" %>

kimuramitai_profile_image_blob: <%= ActiveStorage::Blob.fixture filename: "users/avatars/default.jpg" %>

# article
article4_thumbnail_blob: <%= ActiveStorage::Blob.fixture filename: "articles/ogp_images/test.jpg" %>
6 changes: 6 additions & 0 deletions test/fixtures/articles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,9 @@ article3:
body: 本文3
user: komagata
wip: true

article4:
title: サムネイル画像付きのブログ記事
body: サムネイル画像付きのブログ記事
user: komagata
wip: true
27 changes: 27 additions & 0 deletions test/models/image_resizer_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true

require 'test_helper'

class ImageResizerTest < ActiveSupport::TestCase
test '.fit_to!' do
article = articles(:article4)

ImageResizer.new(article.thumbnail).fit_to!(*ImageResizer::OGP_SIZE)

article.thumbnail.blob.analyze unless article.thumbnail.blob.analyzed?
assert_equal({ width: 1200, height: 630 },
article.thumbnail.blob.metadata.slice(:width, :height).symbolize_keys)
end

test '.just_fit_to_size?' do
article = articles(:article4)

image_resizer = ImageResizer.new(article.thumbnail)
width, height = ImageResizer::OGP_SIZE
image_resizer.fit_to!(width, height)

assert image_resizer.just_fit_to_size?(width, height)
assert_not image_resizer.just_fit_to_size?(width + 1, height)
assert_not image_resizer.just_fit_to_size?(width, height + 1)
end
end
33 changes: 33 additions & 0 deletions test/models/image_resizer_transformation_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

require 'test_helper'

class ImageResizerTransformationTest < ActiveSupport::TestCase
setup do
@image_resizer_transformation = ImageResizerTransformation.new
@width = 1200
@height = 630
end

test '.fit_to' do
assert_equal({ resize: "#{@width}x", gravity: :center, crop: "#{@width}x#{@height}+0+0!" },
@image_resizer_transformation.fit_to(500, 500, @width, @height))
end

test '.resize return 1200x when width magnification is grater than height magnification' do
assert_equal({ resize: "#{@width}x" }, @image_resizer_transformation.resize(500, 500, @width, @height))
assert_equal({ resize: "#{@width}x" }, @image_resizer_transformation.resize(500, 630, @width, @height))
assert_equal({ resize: "#{@width}x" }, @image_resizer_transformation.resize(1500, 1500, @width, @height))
end

test '.resize return x630 when height magnification is grater than width magnification' do
assert_equal({ resize: "x#{@height}" }, @image_resizer_transformation.resize(1000, 500, @width, @height))
assert_equal({ resize: "x#{@height}" }, @image_resizer_transformation.resize(1200, 500, @width, @height))
assert_equal({ resize: "x#{@height}" }, @image_resizer_transformation.resize(1500, 700, @width, @height))
end

test '.center_crop' do
assert_equal({ gravity: :center, crop: "#{@width}x#{@height}+0+0!" },
@image_resizer_transformation.center_crop(@width, @height))
end
end