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.

Sign up for GitHub

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

Jump to bottom

通知一覧のメンションの合計数を削除 #4390

Merged
merged 1 commit into from
Mar 14, 2022
Merged

通知一覧のメンションの合計数を削除 #4390

merged 1 commit into from
Mar 14, 2022

Conversation

saeyama
Copy link
Contributor

@saeyama saeyama commented Mar 13, 2022 •
edited
Loading

Issue

概要

  • 通知一覧のメンションタブの合計数を確認するシチュエーションが無いとのことで、合計数の箇所を削除しました。

参考

下記のプルリクを参照の上、不要なコードの箇所を削除しました。

変更確認方法

  1. ブランチ feature/remove-total-number-of-mention-in-notification-listをローカルに取り込む
  2. rails s でローカル環境を立ち上げる
  3. ログイン(ローカル上ではkimuraでログインしました)
  4. 右上の通知をクリックして、全ての通知をクリック

変更前

before

変更後

after

@saeyama saeyama self-assigned this Mar 13, 2022
@saeyama saeyama marked this pull request as ready for review March 13, 2022 01:42
@saeyama
Copy link
Contributor Author

saeyama commented Mar 13, 2022

@aim2bpg さん、お手隙の際にレビューよろしくお願いします🙏

@saeyama saeyama requested a review from aim2bpg March 13, 2022 01:43
aim2bpg
aim2bpg approved these changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@aim2bpg aim2bpg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@saeyama さん、レビュー依頼ありがとうございます。
私はOKです🙆‍♀️チームリーダーレビューに進められてください👍

以下、蛇足となりますが、こんな考え方もあるんだな程度でご参考となさってください🙇🏻‍♂️

Issueには仕様(変更内容)の形で伝えられることが多々ありまして、PRに要求の背景みたいなものを調べて一言書いてあると、なぜ、この変更をしなければならないかが明確となり、変更のモチベーションも上がります。私は作業者ではなく、エンジニアなのでそこまでちゃんと考えてこのPRを書いているんですよってことがPRを見れば伝わってきますよね。PR参照先の#3925 でParuさんが仰っているようなことです。

今回のケースでは、参照先のPR→Issueを辿ってみると「この数を確認する機会はなく不要だったので削除。」と#4390 に書いてありましたので、あ〜だから削除するんだと私は腑に落ちました。(追記)#4355 にも書いてありましたね、すみません😅

たまに厄介なものだと、そもそも変更は不要だったとか、いや別の実現方法もあるよね、みたいなこともありますので、なるべく変更する前にプロダクトオーナーに質問するなりして明らかにしておきたいですよね。まぁ、あまり気にしすぎず、気にかけておかれる程度でも良いかもな話でした。...長文失礼いたしました🙏

@saeyama saeyama requested a review from komagata March 14, 2022 12:57
@saeyama
Copy link
Contributor Author

saeyama commented Mar 14, 2022

@aim2bpg さん、レビューありがとうございます！
おっしゃる通り、なぜ、この変更をすることになったのか記載をしていなかったので、レビューをする方からするとわかりづらかったですね。ご指摘ありがとうございます🙏
意図されている対応と異なる点があるかもしれませんが、概要欄を以下に修正しました。
事足りていない点があれば、またご指摘頂ければと思います！

  • 通知一覧のメンションタブの合計数を確認するシチュエーションが無いとのことで、合計数の箇所を削除しました。

@komagata さん、お手隙の際にレビューよろしくお願いします🙇‍♀️

komagata
komagata reviewed Mar 14, 2022
View reviewed changes
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 6965ca2 into main Mar 14, 2022
@komagata komagata deleted the feature/remove-total-number-of-mention-in-notification-list branch March 14, 2022 14:56
@github-actions github-actions bot mentioned this pull request Mar 14, 2022
Merged
32 tasks
@aim2bpg
Copy link
Contributor

aim2bpg commented Mar 15, 2022

@saeyama さん、ご対応ありがとうございます🙏
昨日は余計なこと書きすぎたな〜とちょっと反省しました😅 この後もチーム開発、頑張っていきましょう😄

意図されている対応と異なる点があるかもしれませんが、概要欄を以下に修正しました。
事足りていない点があれば、またご指摘頂ければと思います！

通知一覧のメンションタブの合計数を確認するシチュエーションが無いとのことで、合計数の箇所を削除しました。

はい、バッチリです！

@saeyama
Copy link
Contributor Author

saeyama commented Mar 15, 2022

@aim2bpg さん、確認ありがとうございます！
気づけないことも多々あるので、ご指摘頂けて有り難いです〜！
わからないことがあった時は、色々お伺いするかもしれないので、、その際はよろしくお願いします🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@komagata komagata komagata left review comments

@aim2bpg aim2bpg aim2bpg approved these changes

Assignees

@saeyama saeyama

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@saeyama @aim2bpg @komagata