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

rack-dev-markというgemを追加し開発環境での作業時に目印が出るようにした #4174

Merged
merged 8 commits into from
Feb 16, 2022

Conversation

eatplaynap
Copy link
Contributor

@eatplaynap eatplaynap commented Feb 8, 2022

issue

概要

rack-dev-markというgemを入れて、どの環境でbootpcampアプリが動いているのかを表示できるようにしました。

変更確認方法

  1. ブランチfeature/add-rack-dev-mark-gemをローカルに取り込む
  2. $ rails sでローカル環境を立ち上げる
  3. テスト用ユーザーでログイン

変更前

image.png

変更後

image

@eatplaynap eatplaynap self-assigned this Feb 8, 2022
@eatplaynap eatplaynap marked this pull request as ready for review February 8, 2022 06:35
@eatplaynap eatplaynap requested a review from aim2bpg February 8, 2022 06:36
@eatplaynap
Copy link
Contributor Author

@aim2bpg
お疲れさまです!お手すきのときににレビューお願いいたします:pray:

@aim2bpg
Copy link
Contributor

aim2bpg commented Feb 8, 2022

@eatplaynap さん、Issue対応おつかれさまです。

developmentが表示されることを確認しました。OKです🙆‍♀️
チームリーダーレビューに進めてください〜。

本番環境でdevelopmentが表示されないことの確認としては、以下のように理解しました👍
今回の変更は開発環境にしか及ばない。よって、本番環境でdevelopmentが表示されることはない。
設定ファイル(config) | Railsドキュメント

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

コメントする場所を間違えたかもです🙏
OKです🙆

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

Approveを選択しないと承認されないのですね〜🙏
今、Approveしました。

@aim2bpg
Copy link
Contributor

aim2bpg commented Feb 9, 2022

追伸というか独り言...です。
ステージング環境で確認される際は、developmentではなく、stagingと表示されることが期待値かな〜と思いました😄
念のため、ステージング環境のstaging表示も本Issueのスコープかは確認された方が良いかもしれません🙏

@cafedomancer
Copy link
Contributor

念のため、ステージング環境のstaging表示も本Issueのスコープかは確認された方が良いかもしれません🙏

目的は本番環境での誤操作を防ぐことなので、できれば development, staging のそれぞれで出ているとうれしいです!

@cafedomancer
Copy link
Contributor

cafedomancer commented Feb 9, 2022

あとデフォルトの ribbon の位置だと既存の UI パーツとかぶってしまうので、なにもなさそうな左下か右下に出しておくとよいかもしれません。

追伸: 左下は development だと any_login がいそうなので、消去法で右下ですかね...。

@aim2bpg aim2bpg self-requested a review February 9, 2022 03:01
Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

下記を追記するイメージですかね。(間違ってたら、すみません。。)
一旦、状態を再レビューに変更しました🙏(あれ?再レビューにならなかった)

config/environments/test.rb

  config.rack_dev_mark.enable = true

config/application.rb

module MyApp
  class Application < Rails::Application
    config.rack_dev_mark.theme = [:title, Rack::DevMark::Theme::GithubForkRibbon.new(position: 'right-bottom')]
  end
end

参照元:https://github.com/dtaniwaki/rack-dev-mark/blob/master/THEME.md

Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

レビュー前の状態に戻すには、Request Changesするのですね。
今、レビュー前の状態に戻しました🙏
(初めてなものでたびたび、すみません🔰 )

@eatplaynap
Copy link
Contributor Author

@aim2bpg
レビューありがとうございます〜!たくさんコメントくださってうれしいので全然気になさらないでください!
そして修正方法までご提案ありがとうございます:pray:
自分でもちょっと調べて、実装できたらまた再レビュー依頼しますね〜😄

@cafedomancer
コメントありがとうございます!下記対応します!!

  • 右下に配置させる
  • development, staging のそれぞれで出ていることの確認

@eatplaynap
Copy link
Contributor Author

@aim2bpg @cafedomancer
本日の開発ミーティングでステージング環境のstaging表示について@komagataさんと@machidaさんに相談したところ、現在のブートキャンプアプリではステージング環境と本番環境を区別する仕組みがないので、ステージング環境のstaging表示は本Issueでは対応しなくてよいそうです😅
そのため右下にリボンを配置させるご指摘のみ対応させていただきます🙏

@eatplaynap eatplaynap force-pushed the feature/add-rack-dev-mark-gem branch from 1da71d4 to c3b95a8 Compare February 13, 2022 06:09
@eatplaynap
Copy link
Contributor Author

@aim2bpg
お疲れさまです!
「右下にリボンを配置させる」設定をしたので再レビューお願いします:pray:
↓のissueの影響でテストは落ちています😅

@eatplaynap eatplaynap requested a review from aim2bpg February 13, 2022 23:54
Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん、おつかれさまです。
インデントで悩ましいところがありました。ご確認をお願いします🙏

なお、表示の方はバッチリでした。
左上よりも右下の方が邪魔にならなくていい感じだと思いました😄
image

@@ -27,6 +27,6 @@ class Application < Rails::Application
html_tag.html_safe
end

config.active_storage.resolve_model_to_route = :rails_storage_proxy
config.active_storage.resolve_model_to_route = :rails_storage_proxy
Copy link
Contributor

@aim2bpg aim2bpg Feb 14, 2022

Choose a reason for hiding this comment

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

ここは、

  • デバッグで手を入れた際に意図せず変わってしまったものか
  • インデントが修正されたものか

のどちらでしょうか🤔
どちらにしても今回は修正せずに、後者は別Issueを立てて修正すべきかとも思いました。
駒形さんにお聞きした方が良いかもですね〜。

本ファイルや他のファイルを見てみると、rails new後の追加設定に対して、インデントをスペース1つ分ズラしているという意図を感じますが、単純にズレているのに気付けていないだけかもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aim2bpg
これはズレているのは単純なミスだと考え、ついでなので修正してしまいました🤔

@komagata
この行のインデントが下がっているのは意図されたものでしょうか?
意図されていなかった場合、今回は修正せずに別issueを立てるべきでしょうか?

Copy link
Contributor

@aim2bpg aim2bpg Feb 14, 2022

Choose a reason for hiding this comment

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

これはズレているのは単純なミスだと考え、ついでなので修正してしまいました🤔

了解しました。
今回手を入れると「じゃ、上の方のズレは?」となり、中途半端となってしまいますので、それなら別Issueでやった方が得策かという意図で、コメントさせてもらいました🙏

Copy link
Member

Choose a reason for hiding this comment

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

@eatplaynap こちら意図してないです。ミスです。インデントのずれぐらいは修正しちゃって大丈夫です〜。

Comment on lines +89 to +90
config.rack_dev_mark.enable = true
config.rack_dev_mark.theme = [:title, Rack::DevMark::Theme::GithubForkRibbon.new(position: 'right-bottom')]
Copy link
Contributor

Choose a reason for hiding this comment

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

ここは今回追加したものなので、インデントはこのままでも良いかと思いました😄

Copy link
Member

Choose a reason for hiding this comment

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

@eatplaynap こちら他の部分を書いた時点でインデント間違っていたので修正お願いできるとありがたいです〜。

@eatplaynap eatplaynap force-pushed the feature/add-rack-dev-mark-gem branch from c9d90e9 to 2a80d00 Compare February 15, 2022 04:24
@eatplaynap
Copy link
Contributor Author

@aim2bpg
インデントの件は本Issueでは修正せずに元の状態のままにしました〜!
再レビューお願いいたします:pray:

@eatplaynap eatplaynap requested a review from aim2bpg February 15, 2022 04:29
Copy link
Contributor

@aim2bpg aim2bpg left a comment

Choose a reason for hiding this comment

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

@eatplaynap さん
確認しました。OKですー🙆🏻

@eatplaynap eatplaynap requested a review from komagata February 15, 2022 04:31
@eatplaynap
Copy link
Contributor Author

@komagata
チームメンバーのレビューOKが出たのでお手すきの際ご確認お願いします:pray:

@@ -27,6 +27,6 @@ class Application < Rails::Application
html_tag.html_safe
end

config.active_storage.resolve_model_to_route = :rails_storage_proxy
config.active_storage.resolve_model_to_route = :rails_storage_proxy
Copy link
Member

Choose a reason for hiding this comment

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

@eatplaynap こちら意図してないです。ミスです。インデントのずれぐらいは修正しちゃって大丈夫です〜。

Comment on lines +89 to +90
config.rack_dev_mark.enable = true
config.rack_dev_mark.theme = [:title, Rack::DevMark::Theme::GithubForkRibbon.new(position: 'right-bottom')]
Copy link
Member

Choose a reason for hiding this comment

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

@eatplaynap こちら他の部分を書いた時点でインデント間違っていたので修正お願いできるとありがたいです〜。

@eatplaynap eatplaynap force-pushed the feature/add-rack-dev-mark-gem branch from 2a80d00 to 7576c20 Compare February 16, 2022 05:06
@eatplaynap
Copy link
Contributor Author

@komagata
レビューありがとうございます!config/environments/development.rbconfig/application.rbのインデントを修正しました!

@eatplaynap eatplaynap requested a review from komagata February 16, 2022 05:46
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.

確認しました、OKですー🙆‍♂️

@komagata komagata merged commit 05bf97e into main Feb 16, 2022
@komagata komagata deleted the feature/add-rack-dev-mark-gem branch February 16, 2022 05:56
@github-actions github-actions bot mentioned this pull request Feb 16, 2022
54 tasks
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