企業個別ページ傘下のユーザー一覧をVueからRailsに変更 #7956

Judeeeee · 2024-07-11T12:20:50Z

Issue

企業個別ページ傘下のユーザー一覧をVueからRailsに変更したい。 #7900

概要

現在は企業ごとのユーザー一覧がVueで実装されています。
本PRでは、以下の変更を行いました。

関連するVueに使われているファイルを削除する。
Vueの代わりに既存のslimを使ってユーザーを表示させる

変更確認方法

1.chore/change-company-user-list-vue-to-railsローカルに取り込む
2. foreman start -f Procfile.devでローカルサーバーを立ち上げる
3. 管理者でログインする(ID: komagata)
4. 企業個別ページ傘下のユーザー一覧ページを確認する

確認事項

ブラウザでの表示に差分がないこと
- MaruMaru Incの企業個別ページにアクセスし、表示が変更前後で異なっていないこと
関連するVueに使われていたファイルの消し忘れがないこと

Screenshot

※最初のcommit(f28668a)でページネーションの対応が漏れていましたが、最新のcommit(88fbb96)時点では変更の前後でブラウザ表示の差分はないです。
以下に示す画像は、88fbb96時点のものです。

変更前

変更後

Judeeeee · 2024-07-11T14:26:15Z

@goruchanchan

お疲れ様です！
お忙しいところ恐縮ですが、こちらのPRのレビューをお願いできますでしょうか👀
ご都合がつかなければ遠慮なく仰っていただけると幸いですー🙏

goruchanchan · 2024-07-12T13:05:57Z

@Judeeeee レビュー依頼ありがとうございます！1週間以内に確認しますので今しばらくお待ちください🙇‍♂️

Judeeeee · 2024-07-12T14:43:07Z

@goruchanchan

ありがとうございます！
急ぎではないので、goruchanchanさんのご都合の良いタイミングで対応いただけると幸いです🙏
引き続きよろしくお願いいたします〜

goruchanchan · 2024-07-14T13:25:00Z

@Judeeeee お疲れ様です！動作確認問題なさそうです！コードも特に気になる点はなかったです！
１点確認なのですが、変更後のページネーションが表示されなくなりましたが、表示件数が変更前後で異なっていると思っておけば良いでしょうか？

Judeeeee · 2024-07-17T12:40:21Z

@goruchanchan

レビューありがとうございました！
質問の回答が遅くなりました🙇

１点確認なのですが、変更後のページネーションが表示されなくなりましたが、表示件数が変更前後で異なっていると思っておけば良いでしょうか？

これについてはいいえ、という回答になります。
混乱させてしまい本当に申し訳ないのですが、表示件数以前にページネーションの問題を私が見過ごしていたたためこの様な自体になっているとご認識いただければと思います🙇🙇
私の最初のコメントが大嘘だった、というオチです😓

現在は、コメントのscreenshot差し替え&修正commitにより修正しました。
お手数おかけして恐縮なのですが、最新の状態を取り込み、以下の観点をローカル環境で再度ご確認いただけると幸いです🙏

MaruMaru Inc.の企業ユーザー一覧ページにアクセスする
企業の研修ユーザーが総数29なのを確認する(ユーザーが25以上でページネーションが表示される)
ページネーションが上下2つに存在することを確認する

よろしくお願いいたします…！

goruchanchan · 2024-07-17T14:16:12Z

@Judeeeee ご対応ありがとうございます🙇‍♂️

本Issueで対応すべき内容かの判断つかなかったのですが、main ブランチと下記部分が異なっているように見えました🙇‍♂️対応必要か判断いただきたいです🙇‍♂️

main ブランチ
chore/change-company-user-list-vue-to-rails ブランチ

また、コンフリクトが発生しているようですのでご対応お願いします🙇‍♂️

Judeeeee · 2024-07-17T14:22:31Z

@goruchanchan

早速のレビューありがとうございます！
確かに仰る通り、表示が違っているように見えますね🧐
私のローカルでも再度確認して、komagataさんに確認とろうと思います🫡

Judeeeee · 2024-07-26T12:26:43Z

@komagata @machida

お疲れ様です！
デザインの件で相談させてください🙏

VueからRailsへの変更に伴い、レビューいただいたコメントを踏まえて修正を進めています。
企業ユーザーページ一覧の表示は、ユーザー一覧ページと整合性をとった方が良いように思うのですが、変更前のVueでの表示と同一にすべきでしょうか？🤔

今回気にしているポイントは以下の2つです。

ユーザーのDiscordアイコン
- ユーザー一覧ページの表示: Discordアイコンだけのユーザーは存在しない
ページネーション先頭の矢印(<<)表示
- ユーザー一覧ページの表示: なし

🖼️修正前の企業一覧ページ(Vueを使っての表示)

🖼️ユーザー一覧ページ

また、ページをロードした際の「ロード中…」の読み込み画面についても対応すべきか判断に迷っているので、ご教示いただけると幸いです🙇

komagata · 2024-08-03T07:21:44Z

@machida

ユーザーのDiscordアイコン

これってどっちが新しい（合わせるべき）仕様でしたっけ？

@Judeeeee

ページネーション先頭の矢印(<<)表示

基本的には矢印の表示がなくなるのではなく、リンクではなくなるようにしたいです。
しかし、kaminariで実装した時にすごく大変なのであればやらなくて良いです。

machida · 2024-08-03T07:25:40Z

@Judeeeee @komagata discordのアイコンだけのユーザーは無しでお願いします🙏

ref: #7956 (comment)

Judeeeee · 2024-08-15T06:17:35Z

@goruchanchan

お疲れ様です！
時間が経っているので、改めて観点をまとめさせていただきました🙏
お手数ですが、レビューをお願いいたします🙇

"管理者"タグの表示
- conflictファイルを修正したことで修正済
ページネーション
- 変更前後で余白の差分を無くした
- ページネーション先頭の矢印(<<)表示を非リンク化(ref: komagataさんのコメント)
Discordアイコン
- 既存の問題。ユーザー登録情報の更新で、discord_profilesテーブルのtimes_urlに空文字が入ってしまうことが原因。本PRでは対応しない。

以上を踏まえて、変更前後の差分を画像比較ツールで確認したので、参考にしていただければと思います🙇

変更前後のブラウザ表示比較

2024-08-15.14.46.13.mov

goruchanchan · 2024-08-19T13:11:47Z

@Judeeeee まとめありがとうございます！また比較ツールも助かりました！とても確認しやすいと思いました！コードも気になる点は特になかったです。

１点だけ細かくて申し訳ないのですが、下記の名前？部分のフォント色が変更差分があるような気がしましたが、こちらは変更後のものが正という理解で良いでしょうか？

Judeeeee · 2024-08-20T08:23:33Z

@goruchanchan

こちらこそレビューありがとうございました！
承知しました🫡
確かに名前フォント色の差分がありますね👀

@komagata @machida

ユーザー一覧ページに合わせて、文字色は現状の黒色細字を正と考えていますが、この認識で問題ないでしょうか？

ユーザー名の文字色はグレイ

goruchanchan

@Judeeeee 遅くなり申し訳ないです🙇‍♂️差分ツール等とても参考になりました！LGTMと思います！！

Judeeeee · 2024-08-23T12:51:21Z

@goruchanchan

とんでもないです！丁寧にレビューいただきありがとうございました！
自分では気づけないところが多くあったので、goruchanchanさんにレビュー依頼できて助かりました🙏

Judeeeee · 2024-08-23T12:59:01Z

@komagata
お疲れ様です！こちらメンバーからApproveいただいたので、お手隙にレビューお願いいたします🙇

sinsoku · 2024-08-25T05:02:32Z

app/views/companies/users/_user_list.html.slim

+      - users.each do |user|
+        = render 'users/user', user:


ここは Rendering a collection of partials を参考に、collection オプションを指定した方がシンプルに書けて良いかなと思いました。
たぶん、こんな感じで書けると思います。

Suggested change

- users.each do |user|

= render 'users/user', user:

= render partial: 'users/user', collection: users

collectionオプションを知らなかったので勉強になりました🙇
61e03a7で修正しました！

sinsoku · 2024-08-25T05:07:25Z

app/views/companies/users/index.html.slim

+  .page-body
+    .container
+      .users
+        - if [email protected]?


if vs unless を参考に if ! のような If + 否定の条件は、unless にした方が良いです。

Suggested change

- if !@users.empty?

- unless @users.empty?

RuboCop のstyleガイドに記述があるんですね！
勉強になります👀

CIを実行したところRuboCopのStyle/UnlessElseに引っかかったので、ドキュメントを参考にc07fa44で修正いたしました🙏

#7956 (comment)

komagata · 2024-09-12T16:35:27Z

app/views/kaminari/company_user/_first_page.html.slim

@@ -0,0 +1,8 @@
+- # rubocop:disable Rails/OutputSafety


kaminariのviewの変更はここだけ特別扱いするとややこしいので、全体で統一したいです。
大元を変更していただければありがたいです。

@komagata

コメントありがとうございます🙏
8f3c4ceで大元のkaminariのページネーションを変更し、企業個別ページ用に作成していたkaminari/company_user配下のファイルを削除しました。

ローカルでRuboCopを実行した時にはパスしていた🤔

ref: #7956 (comment)

ref: https://github.com/fjordllc/bootcamp/pull/7956/files/9144eb9f48640594d0dcda12c550d570feea2ebd#r1730211184

#7956 (comment)

企業個別ユーザーページだけ特別扱いせず、大元のkaminariのページネーションを変更した - #7956 (comment) - https://github.com/fjordllc/bootcamp/pull/7956/files#r1757220135

komagata · 2024-11-15T04:48:36Z

app/views/companies/users/_user_list.html.slim

@@ -1,8 +1,8 @@
 - if users.present?
  .user-list
-    = paginate users, theme: 'company_user', params: { controller: 'companies/users', action: 'index' }
+    = paginate users, params: { controller: 'companies/users', action: 'index' }


このparams以降って必要でしょうか。

@komagata

レビューありがとうございます🙏
確かに仰る通り、必要ないですね💦
c613f4eで記述を削除しました。

また、2e5d7b6でユーザー一覧のslimを使い回せるので、企業ユーザー用に作成していたslimを削除しましたが、同様のことを最新最新mainの_company_users.html.slimで行っているので、mergeコミットを積む(38c956f)ことで本PRに反映しました。

Merge branch 'main' into chore/change-company-user-list-vue-to-rails

Judeeeee self-assigned this Jul 11, 2024

Judeeeee changed the title ~~[WIP] Vueから既存のslimを利用する形に変更~~ [WIP] 企業個別ページ傘下のユーザー一覧をVueからRailsに変更 Jul 11, 2024

Judeeeee changed the title ~~[WIP] 企業個別ページ傘下のユーザー一覧をVueからRailsに変更~~ 企業個別ページ傘下のユーザー一覧をVueからRailsに変更 Jul 11, 2024

Judeeeee marked this pull request as ready for review July 11, 2024 14:23

goruchanchan self-requested a review July 14, 2024 13:27

Judeeeee force-pushed the chore/change-company-user-list-vue-to-rails branch from fe7bbfa to 7ab083c Compare July 25, 2024 12:03

Judeeeee added a commit that referenced this pull request Aug 15, 2024

最初のページではページネーション先頭の矢印(<<)表示を非リンクにする

a4bd033

ref: #7956 (comment)

goruchanchan approved these changes Aug 23, 2024

View reviewed changes

Judeeeee requested a review from komagata August 23, 2024 12:59

sinsoku reviewed Aug 25, 2024

View reviewed changes

Judeeeee added a commit that referenced this pull request Aug 26, 2024

RuboCopのStyle/UnlessElseとSQLの発行回数を考慮し、present?を採用

c07fa44

#7956 (comment)

Judeeeee force-pushed the chore/change-company-user-list-vue-to-rails branch from 9331df1 to c07fa44 Compare August 26, 2024 13:50

Judeeeee requested a review from sinsoku August 26, 2024 14:39

komagata reviewed Sep 12, 2024

View reviewed changes

Judeeeee added 10 commits November 12, 2024 14:54

Vueから既存のslimを利用する形に変更

8b6cba0

コンフリクトの解消&変更によりページネーションが表示されなくなっていたので修正

770c6f9

ユーザー数を正しく表示できていなかったため、他の表記に合わせてtotal_countを使う

9921264

変更の前後で、ブラウザでの表示が異なっていたため修正

610c50a

CIで落ちていたRuboCop指摘の修正

73da165

ローカルでRuboCopを実行した時にはパスしていた🤔

余白を変更前に合わせる

87811c8

最初のページではページネーション先頭の矢印(<<)表示を非リンクにする

d3fcf59

ref: #7956 (comment)

可読性を考慮し、文字数が少ないcollectionオプションを指定

7a08baa

ref: https://github.com/fjordllc/bootcamp/pull/7956/files/9144eb9f48640594d0dcda12c550d570feea2ebd#r1730211184

RuboCopのStyle/UnlessElseとSQLの発行回数を考慮し、present?を採用

dcc961f

#7956 (comment)

Judeeeee force-pushed the chore/change-company-user-list-vue-to-rails branch from c07fa44 to 8f3c4ce Compare November 12, 2024 07:13

Judeeeee requested a review from komagata November 12, 2024 07:28

komagata reviewed Nov 15, 2024

View reviewed changes

Judeeeee added 4 commits November 16, 2024 16:18

不必要な記述を削除

c613f4e

ユーザー一覧のslimを使い回せるので、企業ユーザー用に作成していたslimを削除した

2e5d7b6

コンフリクトしたため、最新mainを取り込んでmergeコミットを積む

38c956f

Merge branch 'main' into chore/change-company-user-list-vue-to-rails

nav.pagenationはkaminariのpagenatorで定義してあるので、重複する記述を削除

ba73ddb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

企業個別ページ傘下のユーザー一覧をVueからRailsに変更 #7956

企業個別ページ傘下のユーザー一覧をVueからRailsに変更 #7956

Judeeeee commented Jul 11, 2024 •

edited

Loading

Judeeeee commented Jul 11, 2024

goruchanchan commented Jul 12, 2024

Judeeeee commented Jul 12, 2024

goruchanchan commented Jul 14, 2024

Judeeeee commented Jul 17, 2024 •

edited by goruchanchan

Loading

goruchanchan commented Jul 17, 2024

Judeeeee commented Jul 17, 2024

Judeeeee commented Jul 26, 2024 •

edited

Loading

komagata commented Aug 3, 2024

machida commented Aug 3, 2024

Judeeeee commented Aug 15, 2024 •

edited

Loading

goruchanchan commented Aug 19, 2024

Judeeeee commented Aug 20, 2024

komagata commented Aug 21, 2024

machida commented Aug 21, 2024

Judeeeee commented Aug 21, 2024

goruchanchan left a comment

Judeeeee commented Aug 23, 2024

Judeeeee commented Aug 23, 2024

sinsoku Aug 25, 2024

Judeeeee Aug 26, 2024

sinsoku Aug 25, 2024

Judeeeee Aug 26, 2024

komagata Sep 12, 2024

Judeeeee Nov 12, 2024

komagata Nov 15, 2024

Judeeeee Nov 16, 2024

	- users.each do \|user\|
	= render 'users/user', user:
	= render partial: 'users/user', collection: users

企業個別ページ傘下のユーザー一覧をVueからRailsに変更 #7956

Are you sure you want to change the base?

企業個別ページ傘下のユーザー一覧をVueからRailsに変更 #7956

Conversation

Judeeeee commented Jul 11, 2024 • edited Loading

Issue

概要

変更確認方法

確認事項

Screenshot

変更前

変更後

Judeeeee commented Jul 11, 2024

goruchanchan commented Jul 12, 2024

Judeeeee commented Jul 12, 2024

goruchanchan commented Jul 14, 2024

Judeeeee commented Jul 17, 2024 • edited by goruchanchan Loading

goruchanchan commented Jul 17, 2024

Judeeeee commented Jul 17, 2024

Judeeeee commented Jul 26, 2024 • edited Loading

🖼️修正前の企業一覧ページ(Vueを使っての表示)

🖼️ユーザー一覧ページ

komagata commented Aug 3, 2024

machida commented Aug 3, 2024

Judeeeee commented Aug 15, 2024 • edited Loading

変更前後のブラウザ表示比較

goruchanchan commented Aug 19, 2024

Judeeeee commented Aug 20, 2024

最新main(bd49ba8)のユーザー一覧ページ

komagata commented Aug 21, 2024

machida commented Aug 21, 2024

Judeeeee commented Aug 21, 2024

ユーザー名の文字色はグレイ

goruchanchan left a comment

Choose a reason for hiding this comment

Judeeeee commented Aug 23, 2024

Judeeeee commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judeeeee commented Jul 11, 2024 •

edited

Loading

Judeeeee commented Jul 17, 2024 •

edited by goruchanchan

Loading

Judeeeee commented Jul 26, 2024 •

edited

Loading

Judeeeee commented Aug 15, 2024 •

edited

Loading

最新main(`bd49ba8`)のユーザー一覧ページ