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

メンター向けプラクティスWatch機能の追加 #3402

Merged
merged 4 commits into from
Oct 26, 2021

Conversation

kaiyu-tech
Copy link
Contributor

@kaiyu-tech kaiyu-tech commented Oct 15, 2021

ref #3347

  • 修正後イメージ
    image

  • メンターとしてログイン時に、提出物が必要なプラクティスの個別ページにWatchボタンを表示

  • このプラクティスで新規提出されたら、Watchしているメンターに通知

    • サイト内通知
    • メール通知(設定でONの場合のみ)

@kaiyu-tech
Copy link
Contributor Author

@machida お手すきの際にこちらのデザイン確認をお願いいたします🙏

@kaiyu-tech kaiyu-tech self-assigned this Oct 15, 2021
@kaiyu-tech kaiyu-tech requested review from machida and removed request for machida October 15, 2021 22:08
@machida
Copy link
Member

machida commented Oct 19, 2021

デザイン了解ですー!

@kaiyu-tech kaiyu-tech requested a review from kotakawase October 22, 2021 12:56
@kaiyu-tech
Copy link
Contributor Author

@kawase-k お手すきの際にこちらのレビューをお願いいたします🙏

デザインについては同時進行で対応されるとのことです!

@kotakawase
Copy link
Contributor

@kaiyu-tech さん
レビュー依頼ありがとうございます🙇‍♂️
デザインの件、承知しました!

先ほど手元で確認させていただきました。
プラクティスをWatchしているメンターに対し、課題の新規提出があった際の動作(サイト内通知・メール通知)については問題ないかと思いました👍
(.deliver_laterメソッドを使って非同期でメールを送っている処理を理解するのに時間がかかってしまいました…!)

上記以外で気になる点が2つほどありましたのでご確認していただきたいです!

  • 今回のIssueの要件になっている「提出物が不要のプラクティスには表示しない」について、不要のプラクティスでも表示されてしまっている気がします。
  • 個人的にメンターモードがOFF時にはWatchボタンは表示させない方がいいのかなと思ったのですが、かいゆーさんのご意見をお聞きしたいです。

お手数おかけしますがご確認のほどよろしくお願いいたします🙏

@@ -54,6 +54,9 @@ header.page-header
- status = @practice.status_by_learnings(current_user.learnings)
.js-learning-status(data-practice-id="#{@practice.id}" data-status="#{status}" data-submission="#{@practice.submission}")

- if current_user.mentor?
#js-watch-toggle(data-watchable-id="#{@practice.id}", data-watchable-type='Practice')
Copy link
Contributor

Choose a reason for hiding this comment

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

個人的にメンターモードがOFF時にはWatchボタンは表示させない方がいいのかなと思ったのですが、かいゆーさんのご意見をお聞きしたいです。

自分が担当していたことを想定して補足させていただきます。

.is-only-mentorを使うとON/OFFの切り替え時に表示/非表示ができそうです。
同ファイル内の「+プラクティス作成」でも使われていました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kawase-k
「提出物が不要のプラクティスには表示しない」について、要件に対応するのを失念していました💦
対応するように修正しました!

またアドバイスありがとうございます!おっしゃられる通りだと思います。
メンターモードがOFF時にはWatchボタンを表示しないように修正しました!

再確認お願いします🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaiyu-tech さん
ご対応いただきありがとうございます!!

先ほど確認しました。
いいと思います〜🎉

@kaiyu-tech kaiyu-tech force-pushed the feature/practice_watch_for_mentors_only branch from 65ff936 to 80819bf Compare October 24, 2021 08:55
@kaiyu-tech
Copy link
Contributor Author

@kawase-k レビューありがとうございました!

@machida
Copy link
Member

machida commented Oct 25, 2021

@kaiyu-tech デザイン入れましたー

@machida machida removed their assignment Oct 25, 2021
@kaiyu-tech
Copy link
Contributor Author

@machida デザインありがとうございます!

@komagata お手すきの際にこちらのレビューをお願いいたします🙏

@kaiyu-tech kaiyu-tech marked this pull request as ready for review October 25, 2021 08:39
@kaiyu-tech kaiyu-tech requested a review from komagata October 25, 2021 08:40
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 5ce8b48 into main Oct 26, 2021
@komagata komagata deleted the feature/practice_watch_for_mentors_only branch October 26, 2021 06:56
@github-actions github-actions bot mentioned this pull request Oct 26, 2021
11 tasks
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.

4 participants