bootcamp
bootcamp copied to clipboard
fjordllc
企業情報ページに管理者のみが登録・閲覧・編集できるメモを実装
Issue
概要
企業情報ページに管理者のみが登録・閲覧・編集できるメモを実装する。
主なユースケース
- 研修全体の管理者（研修に問題があった時連絡する人）を記載する
- 企業特有の問題（内定者研修でのみ利用するなど）を記載する
変更確認方法
-
feature/add-admin-memo-to-company-infoをローカルに取り込む - bootcamp を起動する
foreman start -f Procfile.dev - 管理者でログインする
- 任意の企業情報ページを開く
http://localhost:3000/companies/id - 管理者向けメモを登録し、登録したメモが表示されるかを確認する。
- 管理者以外でログインして、上記ページで管理者向けメモが表示されないことも確認する。
Screenshot
企業詳細画面 http://localhost:3000/companies/id
変更前
変更後
企業編集画面 http://localhost:3000/admin/companies/id/edit
変更前
変更後
@88-99 (cc: @machida ) お疲れ様です。 今からレビューしようと思うのですが、動作確認時に気になる点がありました。
企業メモが以下のような状態になっています。
a-placeholderの適用を止めるとなくなりました。こちらの実装が意図されたものかどうかお伺いしたいです。
https://github.com/fjordllc/bootcamp/blob/a66fd1f702609994d41cd440b4dc07acd6a1078f/app/views/companies/show.html.slim#L33
https://github.com/fjordllc/bootcamp/blame/a66fd1f702609994d41cd440b4dc07acd6a1078f/app/views/companies/show.html.slim#L33
https://github.com/fjordllc/bootcamp/assets/106903482/1acb6409-7f7f-4265-9dec-a923eb300798
@reckyy
ご確認いただきまして、ありがとうございます。
a-placeholderの適用を止めるとなくなりました。こちらの実装が意図されたものかどうかお伺いしたいです。
こちらの実装というのは、メモの内容が点滅する動作のことで合っていますでしょうか？念の為。 もしそうでしたら、@machida さんにはお聞きしなかったのですが、私の見解ですと意図されたものであると判断しました。
- 管理者として企業ページを開くと管理者向けメモが点滅しますので、デザインの仕上がり確認時などにも目にとまる
- 管理者向けメモだということを目立たせる仕様にしている
のではないかと考えました。 デザインしていただいたときに、おもしろいと思ったので一応私もコードを確認いたしました。 以下のファイルの9行目で町田さんがanimationを定義されています。 https://github.com/fjordllc/bootcamp/blame/main/app/javascript/stylesheets/atoms/_a-placeholder.sass 【参考】 https://developer.mozilla.org/ja/docs/Web/CSS/animation
PRでは、管理者向けメモが点滅することに触れていませんでしたので、GIFを追加しました🙏 いかがでしょうか？
@88-99 @reckyy a-placeholder は読み込み待ちの時に表示するもので、読み込みが完了した後は消える必要があります。もしそのように使われていない場合は、dawaさんが a-placeholder の意図や存在理由を勘違いしている可能性があります。
@88-99 @machida ご返信ありがとうございます。
dawaさんへの返信
こちらの実装というのは、メモの内容が点滅する動作のことで合っていますでしょうか？念の為。
おっしゃる通りです。曖昧な言及で申し訳ございません。 採用された意図の共有もありがとうございます。 @machida さんのご説明を踏まえて再度ご返信をお待ちしております。 🙇
machidaさんへの返信
自分もその使用理由は知らなかったです。ご共有ありがとうございます。
@machida
a-placeholder は読み込み待ちの時に表示するもので、読み込みが完了した後は消える必要があります。もしそのように使われていない場合は、dawaさんが a-placeholder の意図や存在理由を勘違いしている可能性があります。
勘違いしてました。こちらお手隙の時に修正していただけませんでしょうか🙏
@reckyy
おっしゃる通りです。曖昧な言及で申し訳ございません。
一応確認させていただいただけですので。
仕様に関して確認が大事でした😓ご質問くださり、ありがとうございました🙇♂️
@machida a-placeholder を削除しました。 https://github.com/fjordllc/bootcamp/pull/7401/commits/8f5d6320fe58e9f99f144be2d371f8e7b6f2e0ae
@reckyy お手数ですが、レビューお願いいたします🙏
ご確認いただき、ありがとうございます！ コメント：https://github.com/fjordllc/bootcamp/pull/7401#pullrequestreview-1965264220
5394a65 は「i18n追加」などに改めた方がいいかもと思いました。このコミットで、メモを実装したと勘違いするかもしれないので。
そうですね。修正します。
こちらはdawaさんが実装したわけではないですが、管理者として情報変更した後に企業一覧に遷移するのはUI的に微妙かな〜と感じました。 メモなら尚更、メモ記入後も企業の情報を開いたままのほうがいいはずです。このPRで変更するのもありかなと思いましたが。。（もしくは別Issue？good first issueにはよさそう。） dawaさんのご意見をお伺いしたいです。
ご意見いただき、ありがとうございます。 私としても、一覧画面に行くのは少し違和感がありました。更新後に企業の詳細(個別)ページに遷移する方が、更新内容が確認しやすい気はしています。同じようなケースで、日報の内容を更新するとどうなるかを確認したところ、詳細ページに遷移しました。
- 一覧画面に遷移した場合は、同じ企業の個別ページをまた開きたいと思った時、探して選択する必要がある
- 企業の個別ページに遷移した場合は、一覧画面のボタンを押せば一覧画面に行ける
駒形さんにご意見をお伺いしたいです。
@komagata 上記についてご意見を伺いたいです。
- 企業情報更新後の遷移先を、一覧画面と詳細画面のどちらが良いのか。
- もし詳細画面に遷移するよう変更する場合、別Issueを立てた方がよろしいでしょうか。
お願いいたします。
https://github.com/fjordllc/bootcamp/pull/7401#issuecomment-2026592235
@machida こちら町田さんはどうおもわれますか？
@komagata @88-99 @reckyy 編集後は企業個別ページに遷移に変更したいと思います。企業メモがページ遷移なく編集ができるようになったら、また変更をするかもですが、今回はそのようにしたいと思います。この変更も、同じIssueでお願いしたいです。
@reckyy ご指摘いただいた2点、修正しました。ご確認よろしくお願いいたします🙏
- コミットメッセージの変更 #7401 (review) https://github.com/fjordllc/bootcamp/pull/7401/commits/5940fae6d98993264a94408fe5403a3d1d07a866
- テストの簡素化 #7401 (comment)