-
Notifications
You must be signed in to change notification settings - Fork 72
メンター用プロフィール公開設定のON/OFFスイッチの反転と属性名変更 #8553
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
base: main
Are you sure you want to change the base?
Conversation
81a78f8
to
0748586
Compare
0748586
to
c5d6ae6
Compare
@kushimegu |
@sekito1107 |
動作は確認できたのですが、コードで気になったことがあったのでコメントしました! |
@kushimegu |
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.
すみません、pendingになっていることに気づかず…。Submit reviewしたので見えるかなと思います。確認をお願いします。
def down | ||
execute "UPDATE users SET show_mentor_profile = NOT show_mentor_profile" | ||
end | ||
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.
マイグレーションファイルを書き換えるわけにはいかないと思うのでただの意見なんですが、
execute
でSQLを書くよりupdate_all
した方がコードが短くなるのでは- Rails Guideでは
reversible
メソッドとchange
を使う代わりにup/down
も使えると書かれているのでreversible
を使えるならそちらで良いのかも
と思ったのですが、どうでしょうか?
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.
ご指摘ありがとうございます!
マイグレーションファイルについては、本番環境で既に運用されている場合は書き換えずに新しいマイグレーションを追加するほうが良いと思いますが、今回は開発ブランチでの作業なので、書き換えでも問題ないと判断しました👍
また、update_all を使う方がより Rails っぽくて読みやすいと感じたので、そちらを取り入れさせていただきました!
reversible についても確かにその書き方は可能だと思いますが、「reversible を優先すべき」というよりは、処理の意図が伝わりやすい書き方を選ぶべきだと考えています。
今回のようにシンプルな反転処理であれば、up/down を使って明示的に書いたほうが分かりやすいかなと思ったのですが、いかがでしょうか?🙏
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.
@kushimegu
レビューありがとうございます、修正に伴いコメントしましたのでご確認お願いしますー🙏
def down | ||
execute "UPDATE users SET show_mentor_profile = NOT show_mentor_profile" | ||
end | ||
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.
ご指摘ありがとうございます!
マイグレーションファイルについては、本番環境で既に運用されている場合は書き換えずに新しいマイグレーションを追加するほうが良いと思いますが、今回は開発ブランチでの作業なので、書き換えでも問題ないと判断しました👍
また、update_all を使う方がより Rails っぽくて読みやすいと感じたので、そちらを取り入れさせていただきました!
reversible についても確かにその書き方は可能だと思いますが、「reversible を優先すべき」というよりは、処理の意図が伝わりやすい書き方を選ぶべきだと考えています。
今回のようにシンプルな反転処理であれば、up/down を使って明示的に書いたほうが分かりやすいかなと思ったのですが、いかがでしょうか?🙏
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.
マイグレーションファイルについては、本番環境で既に運用されている場合は書き換えずに新しいマイグレーションを追加するほうが良いと思いますが、今回は開発ブランチでの作業なので、書き換えでも問題ないと判断しました👍
捕捉と修正ありがとうございます!
reversible
を使うかup/down
を使うかでどっちが読みやすいかは正直判断できないのですが、特にどちらでも問題ないと思うのでこれでApproveとさせていただきます。
@kushimegu @komagata |
da88d11
to
2966581
Compare
Issue
#8528
概要
UIの直感性向上:
従来のUIでは「プロフィール非公開 (OFF)」という表現が、プロフィールの公開状態を示すものとして直感的ではありませんでした (画像参照)。
これを改善するため、スイッチの表示を「公開」状態を示す「ON」に変更し、UIの直感性を高めました。
文言の修正:
UIの変更に合わせて、プロフィールの公開設定に関する案内文を修正しました。
データモデルの変更:
直感的なUIを実現するため、プロフィールの公開状態を保存する属性名を変更し、既存の真偽値を反転させました。
( 元の属性名
hide_mentor_profile
をshow_mentor_profile
に変更し、真偽値を反転)変更確認方法
feature/invert-profile-visibility-switch
をローカルに取り込むgit checkout feature/invert-profile-visibility-switch
bin/rails db:migrate
でデータベースを更新するforeman start -f Procfile.dev
でローカル環境を立ち上げるkomagata
pass:testtest
でログインするhttp://localhost:3000/
にアクセスし、下部へとスクロールし、メンター顧問 の欄に適切にプロフィールが表示されているのを確認するScreenshot
変更前
変更後