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

通知の対象者を表示 #917

Merged
merged 2 commits into from
Dec 26, 2023
Merged

通知の対象者を表示 #917

merged 2 commits into from
Dec 26, 2023

Conversation

mathsuky
Copy link
Contributor

イベント作成時に通知が飛ぶ人を表示するようにしました。
現状対象グループのメンバーを表示しているのですが,何か例外はありますでしょうか。

@mathsuky
Copy link
Contributor Author

書いてから思ったんですが,既存のイベントが編集されるときはその時の参加欠席に基づいて通知が飛んでるような気がしてきました。

@itt828
Copy link
Member

itt828 commented Dec 6, 2023

PRありがとうございます!
とてもよさそうです:dekadekaarigatou:

既存のイベントが編集されるときはその時の参加欠席に基づいて通知が飛んでるような気がしてきました。

個人的には通知対象者を列挙するということにこだわらずに参加予定者をプレビュー時点で表示する機能ということでいいかなと思いました。

@mathsuky
Copy link
Contributor Author

mathsuky commented Dec 19, 2023

レビューありがとうございます!返信がとても遅くなり,大変申し訳ございません。
参加予定者をプレビュー時点で表示する機能ということになると,現状のコミットからどのような点を変えるべきでしょうか。個人的には,このままでもよいのかなと思っています(意図を汲み取れていなかったらすみません:pray:)

@mathsuky
Copy link
Contributor Author

wasabiさんが提案してくださった内容をもとに,targets→inviteesにしました。
良さそうであればマージします!

@itt828
Copy link
Member

itt828 commented Dec 26, 2023

LGTM
マージしてください!

@mathsuky mathsuky merged commit f1db283 into master Dec 26, 2023
6 checks passed
@mathsuky mathsuky deleted the display-notification-targets branch December 26, 2023 01:47
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.

2 participants