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

ユーザー個別ページ、一覧ページのカウント数一覧にポートフォリオを追加 #7522

Merged
merged 3 commits into from
May 15, 2024

Conversation

unikounio
Copy link
Contributor

@unikounio unikounio commented Mar 12, 2024

Issue

概要

ユーザー個別ページ、一覧ページのカウント数一覧にポートフォリオを追加しました。
また、ユーザー個別ページのカウント数一覧において、カウント数が0の場合はリンクしないよう変更しました。

変更確認方法

  1. feature/add-portfolio-count-to-userをローカルに取り込む
  2. bin/setupを実行
  3. foreman start -f Procfile.devでサーバーを立ち上げる
  4. 任意のアカウントでログインし、/usersにアクセス
  5. 次の2点を確認する
    • 各ユーザーのカウント数一覧に「ポートフォリオ」項目が追加されている
    • カウントの数字部分のリンクが次の状態になっている
      • カウントが0ではない場合
        ユーザー個別ページの対応するタブにリンクにしている。
      • カウントが0の場合
        リンクなし。
  6. /usersからユーザーhatsunoをクリック
  7. 次の2点を確認する
    • カウント数一覧に「ポートフォリオ」項目が追加されている
    • カウントの数字部分のリンクが次の状態になっている
      • カウントが0ではない場合
        対応するタブにリンクにしている。
      • カウントが0の場合
        リンクなし。

Screenshot

変更前

/users

Issue#7517変更前(一覧)

/users/show

Issue#7517変更前(個別)

変更後

/users

Issue#7517変更後(一覧)

/users/show

Issue#7517変更後(個別)

@unikounio unikounio self-assigned this Mar 12, 2024
@unikounio unikounio marked this pull request as ready for review March 12, 2024 14:01
@unikounio
Copy link
Contributor Author

@kurumadaisuke
お疲れ様です!
こちらのPRのレビューをお願いさせていただきたくてご連絡させていただきました🙏
ご対応可能なようでしたら、お手すきの際にでもご確認いただけますと幸いです。
よろしくお願いいたします。

@unikounio unikounio requested a review from kurumadaisuke March 12, 2024 14:04
@kurumadaisuke
Copy link
Contributor

@unikounio
すいませんmm
ちょっと仕事がバタバタしていてまとまった時間を取ることが難しく、、、
他の方でもよろしかったでしょうか🙏

@unikounio unikounio requested review from a-kuroki-gs and removed request for kurumadaisuke March 13, 2024 13:06
@unikounio
Copy link
Contributor Author

@kurumadaisuke さん
お忙しいところすいません!
ご連絡ありがとうございます~🙏✨
別の方にお願いさせていただきますね。

@a-kuroki-gs さん
お疲れ様です!
こちらのPRのレビューをお願いさせていただきたくてご連絡させていただきました🙏
ご対応可能なようでしたら、お手すきの際にでもご確認いただけますと幸いです。
よろしくお願いいたします。

@a-kuroki-gs
Copy link
Contributor

@unikounio
お疲れ様です!
レビュー依頼ありがとうございます。

ただ、大変申し訳ないのですが、現在レビューの時間を取ることが厳しいため、
他の方への依頼をお願いしてもよろしいでしょうか?😭

@unikounio unikounio requested review from hirano-vm4 and removed request for a-kuroki-gs March 15, 2024 04:26
@unikounio
Copy link
Contributor Author

@a-kuroki-gs さん
ご連絡ありがとうございます!🙏✨
ご多用のところ依頼してしまってすいませんでした🙏
別の方にお願いさせていただきます~

@hirano-vm4 さん
お疲れ様です!
こちらのPRのレビューをお願いさせていただきたくてご連絡させていただきました🙏
けだまさんからのご依頼をお断りしたのにこちらからご依頼する形になってしまい申し訳ないのですが、ご対応可能なようでしたら、お手すきの際にでもご確認いただけますと幸いです😅
よろしくお願いいたしますmm

@hirano-vm4
Copy link
Contributor

@unikounio

もちろん大丈夫です🙆お時間数日いただければと思います!よろしくお願いします🙏

@hirano-vm4
Copy link
Contributor

hirano-vm4 commented Mar 17, 2024

@unikounio

お疲れさまです!遅くなりました!早速コードを確認しました😄実装のコードは自体はわかりやすく、良いコードだと感じましたが、1点だけ気になった点を共有したいと思います🙏

ユーザー一覧での表示ズレ

http://localhost:3000/users/

  • 問題ない表示
スクリーンショット 2024-03-17 15 32 47
  • 要素の高さがずれている表示
スクリーンショット 2024-03-17 15 33 02

私の環境に固有の現象かもしれませんが、ブラウザのサイズを変更するとactivity-nameに設定された「ポートフォリオ」という文字が他の文字に比べて多いため、横幅がギリギリになっています。

その結果、ブラウザのウィンドウサイズによっては文字列が2行表示になり、レイアウトが崩れてしまうようです🧐

app/javascript/components/user-activity-counts.vue

    user-activity-count(
      activity-name='ポートフォリオ',
      :activity-count='user.work_count',
      :activity-url='`${user.url}/portfolio`')

固有の問題の可能性もあるので @unikounio さんの環境でも念の為確認していただき、状況によっては町田さんにこのままいくかどうかの確認を入れても良いかも?と感じましたがいかがでしょうか?🙆

確認チェック

  • カウント数一覧に「ポートフォリオ」項目が追加されている
  • カウントの数字部分のリンクが次の状態になっている
  • カウントが1以上の場合、対応するタブにリンクにしている。
  • カウントが0の場合 リンクなし

@unikounio
Copy link
Contributor Author

@hirano-vm4 さん
ご確認ありがとうございます!
ご指摘の点については実は私も気になったので確認していました。
本番環境での動きを見たところ、activity-nameの表示は元からこのようになっているようなので、この実装には意図がある、もしくは整理するとしても別Issueになるだろうと考え、変更せずにいた次第です。

【本番環境での表示】
スクリーンショット 2024-03-17 194238

とはいえ2人とも気になる点ということで、念のため町田さんに確認させていただこうと思います😄

@unikounio
Copy link
Contributor Author

unikounio commented Mar 17, 2024

@machida さん
お疲れ様です!
ユーザー一覧画面での表示について、実装方針の確認をさせてください🙏
現在の実装ではユーザー一覧画面(/users)を表示する際に、ブラウザのサイズ次第で要素名が2列になって表示されるようになっているかと思います。

【本番環境での表示】
スクリーンショット 2024-03-17 194238

これについて本PRでは特段の調整を加えない方針で進めているのですが、この点いかがでしょうか?
お手すきの際にご回答いただけますと幸いです。

3/27 追記
本件は本日のチーム開発MTGにて町田さんご確認済み

@hirano-vm4
Copy link
Contributor

@unikounio
お疲れ様です!もとからそのような状況だったのですね👀コード的には私は問題ないと思いましたので、確認できたらApproveさせてもらいます🙏

@unikounio
Copy link
Contributor Author

unikounio commented Mar 27, 2024

@hirano-vm4 さん
お疲れさまです!
要素名の表示ズレの件について、本日のチーム開発MTGで町田さんに確認していただき、別Issueで進めることとなりました。

お手すきの際にご確認をお願いいたします🙏

Copy link
Contributor

@hirano-vm4 hirano-vm4 left a comment

Choose a reason for hiding this comment

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

@unikounio

確認&Issue作成までお疲れ様です!私の方からは、Approveさせていただきます🙏

またレビュー依頼あればお声かけください!私もお願いすることがあると思いますので、引き続きよろしくお願いいたします🙆

@unikounio
Copy link
Contributor Author

@hirano-vm4 さん
レビューありがとうございました!✨
引き続きよろしくお願いいたします~😄

@unikounio unikounio requested a review from komagata March 28, 2024 09:29
@unikounio
Copy link
Contributor Author

@komagata さん
お疲れ様です!
チームメンバーからApproveをいただきましたので、お手すきの際にレビューをお願いいたします🙏

@@ -61,4 +61,8 @@ def all_countries_with_subdivisions
.to_h
.to_json
end

def link_or_text_for_count(count, path)
Copy link
Member

@komagata komagata Apr 7, 2024

Choose a reason for hiding this comment

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

@unikounio link_to_ifを使ったらいい感じになりそうな気がしました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご提案いただいた形で修正させていただきました!

条件式を!zero?nonzero?のどちらにするか迷ったのですが、真偽値を求めていることが明確になるよう!zero?を用いる形にしました。

Copy link
Member

Choose a reason for hiding this comment

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

link_to_ifを使ったら一行なのでメソッドかする必要はないかもとおもいました~。

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 さん
おっしゃる通りですね😅修正させていただきました!
修正に伴い、helperに関するコミットが不要となったため、コミットの整理も併せて行いました~

@unikounio unikounio force-pushed the feature/add-portfolio-count-to-user branch from 85180cf to 5abc0c1 Compare April 11, 2024 05:29
Copy link
Contributor Author

@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.

@komagata さん
ご確認ありがとうございます!
修正させていただきました。お手すきの際に再度ご確認をお願いします🙏

@@ -61,4 +61,8 @@ def all_countries_with_subdivisions
.to_h
.to_json
end

def link_or_text_for_count(count, path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご提案いただいた形で修正させていただきました!

条件式を!zero?nonzero?のどちらにするか迷ったのですが、真偽値を求めていることが明確になるよう!zero?を用いる形にしました。

@unikounio unikounio force-pushed the feature/add-portfolio-count-to-user branch from 5abc0c1 to f23581e Compare April 24, 2024 15:09
@unikounio unikounio force-pushed the feature/add-portfolio-count-to-user branch from f23581e to 4824550 Compare April 24, 2024 15:38
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 059e89e into main May 15, 2024
2 checks passed
@komagata komagata deleted the feature/add-portfolio-count-to-user branch May 15, 2024 07:48
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.

5 participants