Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/controllers/notifications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ class NotificationsController < ApplicationController

def index
@target = params[:target]
@notifications = UserNotificationsQuery.new(
user: current_user,
target: params[:target],
status: params[:status]
).call.page(params[:page])
end

def show
Expand Down
74 changes: 0 additions & 74 deletions app/javascript/components/Notification.jsx

This file was deleted.

151 changes: 0 additions & 151 deletions app/javascript/components/Notifications.jsx

This file was deleted.

27 changes: 27 additions & 0 deletions app/javascript/notifications_remove_after_open.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
document.addEventListener('DOMContentLoaded', () => {
const notificationPage = document.querySelector('#notifications')
if (!notificationPage) return

const allOpenButton = document.querySelector(
'#js-shortcut-unconfirmed-links-open'
)
if (!allOpenButton) return

allOpenButton.addEventListener('click', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#9184 (review)
を読んでいて思ったのですが、通知に関してはリロードすれば未読通知や「一括で開く」ボタンは消えるのではないかなと思いました(違っていたらすみません)

現状だと、app/javascript/unconfirmed-links-open.jsapp/javascript/notifications_remove_after_open.jsで処理が別れていますが、上記のプルリクエストのようにapp/javascript/unconfirmed-links-open.js内でlocation.reload()を行えばDOMの削除や置き換えが不要になるのではないかなと思いましたがいかがでしょうか?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

コメントありがとうございます!
ページリロードについては検討したのですが、ページをリロードするとすべてを読み込むため、一部差し換えのほうが処理負担がかからないのではないかと思いこの実装にしてみました。

Junさんはこの点についてどう思われますか?
リロードのほうがすっきりするとは思いますので、特に問題なければリロードに差し替えます!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ページリロードについては検討したのですが、ページをリロードするとすべてを読み込むため、一部差し換えのほうが処理負担がかからないのではないかと思いこの実装にしてみました。

確かにその通りだと思います🙋‍♂️
一方で、一括で開くボタンを押した時点で複数タブでページを読み込み始めているので、もう一枚リロードによる読み込みが増えても変わらないのでは?とも思いました。

もう一点理由というか気になった点があり、#9184 で未確認のリンクを一括で開くボタン(UnconfirmedLink.jsx)をバニラJSで実装しており、通知一覧ページのボタンもそれらの修正に含まれます。
本来なら一括で開くボタンの修正は#9184 で行うべきで、このPRで修正しなくて良い箇所だと思いました。安定を取るなら#9184 マージされるのを待っても良い気がします。そのまま進めるなら未読通知を一括で開くボタンを押したときの処理はなるべく#9184 の実装に合わせた方が後々コンフリクトが起きた際も修正しやすいのではと思いました。
ちょっとこのあたり認識が間違っていたら申し訳ありませんが気になったのでコメント致しました🙇‍♂️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

確かにその通りだと思います🙋‍♂️
一方で、一括で開くボタンを押した時点で複数タブでページを読み込み始めているので、もう一枚リロードによる読み込みが増えても変わらないのでは?とも思いました。

この点について改めて考えたのですが、やはりリロードではなく部分更新の方が良いのではと思いました。

例えば、Reactなどのクライアント側で状態を管理する実装を想定した場合、ページ遷移を伴わずに要素を操作するケースでは、リロードではなく、状態更新によって表示を切り替えるのが一般的だと思います。

リロードすれば結果的に画面は更新されますが、それを前提にしてしまうと、クライアント側で状態や表示を制御している意味が薄れてしまうのではと思いました。

本来なら一括で開くボタンの修正は#9184 で行うべきで、このPRで修正しなくて良い箇所だと思いました。

こちらに関してなのですが現在のmainの一括で開くJSの処理app/javascript/unconfirmed-links-open.js‎は以下のようになっています。

document.addEventListener('DOMContentLoaded', () => {
  const allOpenButton = document.querySelector(
    '#js-shortcut-unconfirmed-links-open'
  )
  if (allOpenButton) {
    allOpenButton.addEventListener('click', () => {
      const links = document.querySelectorAll(
        '.card-list-item .js-unconfirmed-link'
      )
      links.forEach((link) => {
        window.open(link.href, '_target', 'noopener')
      })
    })
  }
})

#9184app/javascript/unconfirmed-links-open.js‎を確認したところ、

      setTimeout(() => {
        location.reload()
      }, 100)

が追加されおりjunさんのおっしゃる通り、このファイルの変更の箇所はいらなくなると思います。

しかし現在のmainでは一括処理のみのJSしかない形なので、画面を変更する箇所は必要だと思いました。
またどちらが先にマージされるかはわからないですが、仮にどちらがされてもこのファイルを削除するだけで対応できると思うので、現時点では大きなバッティングや解消が難しいコンフリクトにはならないと考えています!
もし認識がずれていればご指摘いただけると助かります。

document.querySelector('.card-list')?.remove()
allOpenButton.closest('.card-footer')?.remove()
document.querySelector('.a-border-tint')?.remove()

const container = document.querySelector('.page-content')
if (container) {
container.innerHTML = `
<div class="o-empty-message">
<div class="o-empty-message__icon">
<div class="i fa-regular fa-smile"></div>
</div>
<p class="o-empty-message__text">未読の通知はありません</p>
</div>
`
}
})
})
1 change: 1 addition & 0 deletions app/javascript/packs/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ import '../toast.js'
import '../tag.js'
import '../tag_edit.js'
import '../bookmark-button.js'
import '../notifications_remove_after_open.js'

import '../stylesheets/application.sass'

Expand Down
9 changes: 7 additions & 2 deletions app/queries/user_notifications_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class UserNotificationsQuery < Patterns::Query

private

def initialize(relation = Notification.all, user:, target: nil, status: nil)
def initialize(relation = Notification.all, user:, target:, status:)
super(relation)
@user = user
@target = target
Expand All @@ -14,12 +14,17 @@ def initialize(relation = Notification.all, user:, target: nil, status: nil)

def query
latest_notifications = @user.notifications
.by_target(@target)
.by_target(validated_target)
.by_read_status(@status)
.latest_of_each_link

Notification.with_avatar
.from(latest_notifications, :notifications)
.order(created_at: :desc)
end

def validated_target
target = @target&.to_sym
Notification::TARGETS_TO_KINDS.key?(target) ? target : nil
end
end
12 changes: 12 additions & 0 deletions app/views/notifications/_filter_button.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
nav.pill-nav
.container
ul.pill-nav__items
li.pill-nav__item
= link_to '未読',
notifications_path(status: 'unread', target: params[:target]),
class: "pill-nav__item-link #{params[:status] == 'unread' ? 'is-active' : ''}"

li.pill-nav__item
= link_to '全て',
notifications_path(target: params[:target]),
class: "pill-nav__item-link #{params[:status] == 'unread' ? '' : 'is-active'}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

もともとNotifications.jsxで一緒になっていた部分ですが分割されたのですね。わかりやすくなってとても良いと思いました🙋‍♂️

30 changes: 30 additions & 0 deletions app/views/notifications/_notification.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
- read_class = notification.read? ? 'is-read' : 'is-unread'

.card-list-item class=read_class
.card-list-item__inner
.card-list-item__user
= link_to user_path(notification.sender), class: 'card-list-item__user-link' do
= image_tag notification.sender.avatar_url,
class: 'card-list-item__user-icon a-user-icon',
alt: notification.sender.login_name

.card-list-item__rows
.card-list-item__row
.card-list-item-title
.card-list-item-title__start
- if !notification.read?
.a-list-item-badge.is-unread
span 未読

h2.card-list-item-title__title[itemProp="name"]
= link_to notification.path,
class: 'card-list-item-title__link a-text-link js-unconfirmed-link',
itemProp: 'url' do
span.card-list-item-title__link-label = notification.message

.card-list-item__row
.card-list-item-meta
.card-list-item-meta__items
.card-list-item-meta__item
time.a-meta dateTime=notification.created_at
= l(notification.created_at, format: :long)
23 changes: 22 additions & 1 deletion app/views/notifications/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,25 @@ main.page-main
hr.a-border
.page-body
.container.is-md
= react_component('Notifications', isMentor: mentor_login?)
= render 'filter_button'
- if @notifications.empty?
.o-empty-message
.o-empty-message__icon
.i.fa-regular.fa-smile
p.o-empty-message__text
= params[:status] == 'unread' ? '未読の通知はありません' : '通知はありません'
- else
.page-content#notifications
- if @notifications.total_pages > 1
nav.pagination
= paginate @notifications
.card-list.a-card
- @notifications.each do |notification|
= render 'notification', notification: notification

- if current_user.mentor? && params[:status] == 'unread'
= render 'application/unconfirmed_links_open', label: '未読の通知を一括で開く'

- if @notifications.total_pages > 1
nav.pagination
= paginate @notifications
Loading