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

正方形に切り抜くように修正する #7431

Merged
merged 13 commits into from
Mar 29, 2024

Conversation

yocchan-git
Copy link
Contributor

@yocchan-git yocchan-git commented Feb 22, 2024

Issue

概要

変更確認方法

  1. feature/resize-avatar-to-square-120-120をローカルに取り込む
  2. foreman start -f Procfile.devでサーバーを起動
  3. アイコンが設定されてあるユーザでログイン(後から設定してもOKです)
  4. http://localhost:3000/users にアクセスする
  5. 右クリック→新しいタブで画像を開くと名前をつけて画像を保存する
  6. 新しいタブで画像を開くでは、正方形になっているか確認する
  7. 保存した画像の情報を見て、大きさが120 x 120になっているか確認する

Screenshot

同じ画像でリサイズの違いを確認しました

変更前

Image from Gyazo

Image from Gyazo

変更後

Image from Gyazo

Image from Gyazo

@yocchan-git yocchan-git self-assigned this Feb 22, 2024
@yocchan-git yocchan-git force-pushed the feature/resize-avatar-to-square-120-120 branch from 6aa3586 to 888ec80 Compare February 24, 2024 01:17
@yocchan-git
Copy link
Contributor Author

yocchan-git commented Feb 26, 2024

@unikounio さん

レビュアーにさせていただきました!
差分がいまいち分かりにくくなっていますが、お手隙でレビューしていただけますと嬉しいです🙏

レビューしていただきたいファイルはuser.rbだけです😆
他の変更はこちらのPRによるものですので、お気になさらずでお願いいたします

そうなんです、1ファイルだけの修正です🫡

@yocchan-git yocchan-git marked this pull request as ready for review February 26, 2024 11:23
Copy link
Contributor

@unikounio unikounio left a comment

Choose a reason for hiding this comment

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

確認させていただきました~
変更確認手順に沿って確認したところ、「新しいタブで開いた画像」は正方形に、「保存した画像」は120 x 120になっていることが確認できました👍

image

スクリーンショット 2024-02-27 165345

コードもよく整理されていて読みやすかったです!

1点だけ気になった点をお伝えさせていただきます。
変更確認方法の中でユーザー一覧ページのURL(http://localhost:3000/users)をお示しいただいたのですが、Screenshotではユーザーのプロフィールページ(http://localhost:3000/users/#{@user.id})での確認の様子が示されているので、ここを統一していただくともっと良くなるかもしれません。

どちらのページからも同様の結果が確認できましたので、アプリの変更作業自体は問題なしとして、私からはApproveとさせていただきます。

よっちゃんさんの卒業前に一緒に開発できて嬉しかったです😄

@yocchan-git
Copy link
Contributor Author

unioさん、レビューありがとうございました!僕も一緒にできて嬉しいです
ご指摘もありがとうございます。助かります😆

@komagata さん、チームメンバーからapproveいただきました!
こちらのPRからブランチを切っていますので、user.rbだけご確認をお願いいたします🙇‍♂️

また、こちらのPRがマージされたら、rebaseしますので、こちらもレビューいただけますと🙏

else
image_url default_image_path
end
rescue ActiveStorage::FileNotFoundError, ActiveStorage::InvariableError
image_url default_image_path
rescue Vips::Error
avatar.variant(resize_to_fit: [120, 120]).processed.url
Copy link
Member

Choose a reason for hiding this comment

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

rescue句の中でエラーが発生しそうな処理を書くのはやめた方がいいと思います。
さらに外側のbeginなどでキャッチできなくはないですが、一般的に悪い作法とされているので避けた方がいいかなと思います。

@yocchan-git yocchan-git force-pushed the feature/resize-avatar-to-square-120-120 branch from 925b8c5 to 945359d Compare March 8, 2024 00:28
@yocchan-git yocchan-git requested a review from komagata March 8, 2024 02:44
@yocchan-git
Copy link
Contributor Author

yocchan-git commented Mar 8, 2024

@komagata さん

指摘いただいた点の修正しました!
お手隙で再度ご確認いただけますと🙇‍♂️

繰り返しになりますが、見ていただきたいのはuser.rbだけです!

@komagata
Copy link
Member

komagata commented Mar 9, 2024

@yocchan-git コンフリクトの修正をお願いします。
また、#7397 をマージしましたのでrebaseお願いします。

@yocchan-git yocchan-git force-pushed the feature/resize-avatar-to-square-120-120 branch from 945359d to 3ee84fd Compare March 9, 2024 22:18
@yocchan-git
Copy link
Contributor Author

@komagata さん

コンフリクトの解消とrebaseしました。
テストデータについても修正してました...🥲申し訳ございません

お手隙で再度レビューお願いいたします🙇‍♂️

@@ -804,4 +810,34 @@ def category_having_active_practice
def category_having_unstarted_practice
unstarted_practices&.first&.categories&.first
end

def fetch_avatar_size(width, height)
Copy link
Member

Choose a reason for hiding this comment

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

userモデルがfatになりすぎてしまうので別のクラスを作成することを含めて設計してみてください。

単純に別クラスに同名メソッドを切り出すとか、クラスメソッドだけの手続き型のクラスを作ってしまうとかがよくある間違いですが、オブジェクト指向プログラミングのプラクティスにあったように、ゼロからオブジェクト指向で設計してみてください。

下記のPRではOGPの画像のリサイズをやろうとしていますが、もしかしたらこちらと合わせて共通化も考えられるかもしれません。 @wata00913 さんと連絡を取り合ってみるのもいいと思います。

#6483

Copy link
Contributor Author

@yocchan-git yocchan-git Mar 22, 2024

Choose a reason for hiding this comment

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

@komagata さん

ImageResizerクラスを作成しました!
お手隙で再度レビューいただけますと嬉しいです🙇‍♂️

また、送っていただいたOGPのリサイズでも使えるクラスにしました。
レビューよろしくお願いいたします🙏

Copy link
Member

Choose a reason for hiding this comment

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

@yocchan-git

@wata00913 さんとまず連絡をとっていただければありがたいとおもいます。
@wata00913 さんもこの部分を長く作業してきているので話がなく勝手に方針がかわったり、共通化されると混乱されるとおもいます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

承知しました!まずwataさんに確認を取ってみます
ありがとうございます🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata さん

wataさんとも話をして、結局共通化しなくても良くなりました。
それを踏まえてレビューしていただけますと嬉しいです🙇‍♂️
#6483 (comment)

@yocchan-git yocchan-git force-pushed the feature/resize-avatar-to-square-120-120 branch 2 times, most recently from a97a765 to 7352933 Compare March 22, 2024 08:55
@yocchan-git yocchan-git requested a review from komagata March 22, 2024 10:51
@@ -593,11 +593,11 @@ def avatar_url
default_image_path = '/images/users/avatars/default.png'

if avatar.attached?
avatar.variant(resize_to_limit: AVATAR_SIZE, autorot: true, saver: { strip: true, quality: 60 }).processed.url
avatar.variant(resize_to_fill: AVATAR_SIZE, autorot: true, saver: { strip: true, quality: 60 }).processed.url
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resize_to_fillオプションの説明です。(参考)

  1. アスペクト比を維持して拡大(or縮小)する
  2. 大きくなった場合は真ん中で切り取る

ざっくりこのようなオプションです。😁
気になる点ありましたら、ぜひコメントください!

よろしくお願いいたします🙇

@yocchan-git yocchan-git force-pushed the feature/resize-avatar-to-square-120-120 branch from 32efd05 to 2f350dd Compare March 28, 2024 12:10
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

@yocchan-git めっちゃすっきりした仕様になってよかったですね。
確認させていただきました。OKです~👌

@komagata komagata merged commit fd19b2a into main Mar 29, 2024
2 checks passed
@komagata komagata deleted the feature/resize-avatar-to-square-120-120 branch March 29, 2024 16:06
@github-actions github-actions bot mentioned this pull request Mar 28, 2024
15 tasks
@yocchan-git
Copy link
Contributor Author

ありがとうございます!

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.

3 participants