From b21f6e1a6e428caff9b7c7cbccc876fb1f348765 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 1 Nov 2025 16:57:17 +0100 Subject: [PATCH 1/7] Group notifications for the same card as they show up --- app/assets/stylesheets/notifications.css | 7 +++ app/helpers/notifications_helper.rb | 5 +- .../notifications_tray_controller.js | 57 +++++++++++++++++++ .../notifications/_notification.html.erb | 1 + app/views/notifications/_tray.html.erb | 2 +- app/views/notifications/trays/show.html.erb | 3 +- 6 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 app/javascript/controllers/notifications_tray_controller.js diff --git a/app/assets/stylesheets/notifications.css b/app/assets/stylesheets/notifications.css index 878eb25c5..b50e35a2c 100644 --- a/app/assets/stylesheets/notifications.css +++ b/app/assets/stylesheets/notifications.css @@ -40,6 +40,13 @@ } } + /* Grouped notifications + /* ------------------------------------------------------------------------ */ + + .notification--grouped { + border: 3px solid red; + } + /* Read items /* ------------------------------------------------------------------------ */ diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index c557624be..69eab11a6 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -20,7 +20,10 @@ module NotificationsHelper end def notification_tag(notification, &) - tag.div id: dom_id(notification), class: "tray__item tray__item--notification", data: { navigable_list_target: "item" } do + tag.div id: dom_id(notification), class: "tray__item tray__item--notification", data: { + navigable_list_target: "item", + notifications_tray_target: "notification", + card_id: notification.card.id, timestamp: notification.created_at.to_i } do concat( link_to(notification, class: [ "card card--notification", { "card--closed": notification.card.closed? }, { "unread": !notification.read? } ], diff --git a/app/javascript/controllers/notifications_tray_controller.js b/app/javascript/controllers/notifications_tray_controller.js new file mode 100644 index 000000000..b356c4b20 --- /dev/null +++ b/app/javascript/controllers/notifications_tray_controller.js @@ -0,0 +1,57 @@ +import { Controller } from "@hotwired/stimulus" + +export default class extends Controller { + static targets = [ "notification", "hiddenNotifications" ] + static classes = [ "grouped" ] + + notificationTargetConnected(notification) { + this.#groupNotificationIfNeeded(notification) + } + + #groupNotificationIfNeeded(notification) { + const groupedNotifications = this.#groupedNotificationsFor(notification) + + if (groupedNotifications.length > 1) { + this.#renderGroup(groupedNotifications) + } + } + + #groupedNotificationsFor(notification) { + const cardId = notification.dataset.cardId + return this.notificationTargets + .filter(notification => notification.dataset.cardId === cardId) + .sort((notification_1, notification_2) => parseInt(notification_1.dataset.timestamp) - parseInt(notification_2.dataset.timestamp)) + } + + #renderGroup(groupedNotifications) { + this.#showAsGrouped(groupedNotifications[0], groupedNotifications.length) + groupedNotifications.slice(1).forEach(notification => this.#hideInGroup(notification)) + } + + #showAsGrouped(notification, size) { + notification.classList.add(this.groupedClass) + this.#showGroupedNotification(notification) + this.#setGroupCount(notification, size) + } + + #hideInGroup(notification) { + this.#hideGroupedNotification(notification) + this.#setGroupCount(notification, "") + } + + // We use a hidden container instead of hiding the notifications directly so that the CSS that sort the + // tray element indexes doesn't get messed up. + #showGroupedNotification(notification) { + if (notification.parentElement === this.hiddenNotificationsTarget) { + this.hiddenNotificationsTarget.parentElement.insertBefore(notification, this.hiddenNotificationsTarget) + } + } + + #hideGroupedNotification(notification) { + this.hiddenNotificationsTarget.appendChild(notification) + } + + #setGroupCount(notification, count) { + notification.querySelector("[data-group-count]").textContent = count + } +} diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index b945960b8..49abafcb6 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -3,6 +3,7 @@
<%= render "notifications/notification/#{notification.source_type.underscore}/header", notification: notification %> +
diff --git a/app/views/notifications/_tray.html.erb b/app/views/notifications/_tray.html.erb index a009835a4..d2fa40a5d 100644 --- a/app/views/notifications/_tray.html.erb +++ b/app/views/notifications/_tray.html.erb @@ -7,7 +7,7 @@ data-navigable-list-actionable-items-value="true" data-navigable-list-reverse-navigation-value="true" data-action="keydown->navigable-list#navigate dialog:show@document->navigable-list#reset turbo-visit->navigable-list#reset keydown.esc->dialog#close:stop click@document->dialog#closeOnClickOutside"> - <%= turbo_frame_tag "notifications", src: tray_notifications_path, refresh: "morph" %> + <%= turbo_frame_tag "notifications", src: tray_notifications_path, refresh: "morph", data: { controller: "notifications-tray", notifications_tray_grouped_class: "notification--grouped" } %>
diff --git a/app/views/notifications/trays/show.html.erb b/app/views/notifications/trays/show.html.erb index f61c52d51..a4700253b 100644 --- a/app/views/notifications/trays/show.html.erb +++ b/app/views/notifications/trays/show.html.erb @@ -1,3 +1,4 @@ -<%= turbo_frame_tag "notifications" do %> +<%= turbo_frame_tag "notifications", data: { controller: "notifications-tray", notifications_tray_grouped_class: "notification--grouped" } do %> <%= render partial: "notifications/notification", collection: @notifications, cached: true %> + <% end %> From 89476245243a63a826d9f4a3063ddb76973bd83f Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 1 Nov 2025 17:31:28 +0100 Subject: [PATCH 2/7] Optimize: group on the initial rendering of the tray once, and then start grouping dynamically Performance optimization to save loops --- .../notifications_tray_controller.js | 19 +++++++++++++++---- app/views/notifications/_tray.html.erb | 2 +- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/javascript/controllers/notifications_tray_controller.js b/app/javascript/controllers/notifications_tray_controller.js index b356c4b20..685df6f02 100644 --- a/app/javascript/controllers/notifications_tray_controller.js +++ b/app/javascript/controllers/notifications_tray_controller.js @@ -4,11 +4,22 @@ export default class extends Controller { static targets = [ "notification", "hiddenNotifications" ] static classes = [ "grouped" ] - notificationTargetConnected(notification) { - this.#groupNotificationIfNeeded(notification) + connect() { + this.group() } - #groupNotificationIfNeeded(notification) { + group() { + this.notificationTargets.forEach(notification => this.#groupNotification(notification)) + this.grouped = true + } + + notificationTargetConnected(notification) { + if (this.grouped && notification.parentElement !== this.hiddenNotificationsTarget) { + this.#groupNotification(notification) + } + } + + #groupNotification(notification) { const groupedNotifications = this.#groupedNotificationsFor(notification) if (groupedNotifications.length > 1) { @@ -40,7 +51,7 @@ export default class extends Controller { } // We use a hidden container instead of hiding the notifications directly so that the CSS that sort the - // tray element indexes doesn't get messed up. + // tray element indexes doesn't get messed up with the child positions changing. #showGroupedNotification(notification) { if (notification.parentElement === this.hiddenNotificationsTarget) { this.hiddenNotificationsTarget.parentElement.insertBefore(notification, this.hiddenNotificationsTarget) diff --git a/app/views/notifications/_tray.html.erb b/app/views/notifications/_tray.html.erb index d2fa40a5d..6a4cc1e6a 100644 --- a/app/views/notifications/_tray.html.erb +++ b/app/views/notifications/_tray.html.erb @@ -7,7 +7,7 @@ data-navigable-list-actionable-items-value="true" data-navigable-list-reverse-navigation-value="true" data-action="keydown->navigable-list#navigate dialog:show@document->navigable-list#reset turbo-visit->navigable-list#reset keydown.esc->dialog#close:stop click@document->dialog#closeOnClickOutside"> - <%= turbo_frame_tag "notifications", src: tray_notifications_path, refresh: "morph", data: { controller: "notifications-tray", notifications_tray_grouped_class: "notification--grouped" } %> + <%= turbo_frame_tag "notifications", src: tray_notifications_path, refresh: "morph", data: { controller: "notifications-tray", notifications_tray_grouped_class: "notification--grouped", action: "turbo:frame-render->notifications-tray#group" } %>
From 8400bb0a82018ae9c0d616d3883e5a6284e9d4e2 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 1 Nov 2025 17:49:48 +0100 Subject: [PATCH 3/7] Optimize: only do one loop to group the cards on the initial grouping There can be many unread notifications. We want to keep the complexity lineal. --- .../notifications_tray_controller.js | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/app/javascript/controllers/notifications_tray_controller.js b/app/javascript/controllers/notifications_tray_controller.js index 685df6f02..12077425b 100644 --- a/app/javascript/controllers/notifications_tray_controller.js +++ b/app/javascript/controllers/notifications_tray_controller.js @@ -9,7 +9,15 @@ export default class extends Controller { } group() { - this.notificationTargets.forEach(notification => this.#groupNotification(notification)) + const notificationsByCardId = this.#groupNotificationsByCardId() + + for (const cardId in notificationsByCardId) { + const notifications = notificationsByCardId[cardId] + if (notifications.length > 1) { + this.#renderGroup(notifications) + } + } + this.grouped = true } @@ -19,6 +27,18 @@ export default class extends Controller { } } + #groupNotificationsByCardId() { + const notificationsByCardId = {} + + this.notificationTargets.forEach(notification => { + const cardId = notification.dataset.cardId + notificationsByCardId[cardId] ||= [] + notificationsByCardId[cardId].push(notification) + }) + + return notificationsByCardId + } + #groupNotification(notification) { const groupedNotifications = this.#groupedNotificationsFor(notification) @@ -31,10 +51,11 @@ export default class extends Controller { const cardId = notification.dataset.cardId return this.notificationTargets .filter(notification => notification.dataset.cardId === cardId) - .sort((notification_1, notification_2) => parseInt(notification_1.dataset.timestamp) - parseInt(notification_2.dataset.timestamp)) } #renderGroup(groupedNotifications) { + groupedNotifications.sort((notification_1, notification_2) => parseInt(notification_1.dataset.timestamp) - parseInt(notification_2.dataset.timestamp)) + this.#showAsGrouped(groupedNotifications[0], groupedNotifications.length) groupedNotifications.slice(1).forEach(notification => this.#hideInGroup(notification)) } From b5c25cce2b0269c8d61e23b0a67342c7d1414422 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sun, 2 Nov 2025 00:06:23 +0100 Subject: [PATCH 4/7] Redundant concat --- app/helpers/notifications_helper.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 69eab11a6..51ff30da0 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -23,14 +23,14 @@ module NotificationsHelper tag.div id: dom_id(notification), class: "tray__item tray__item--notification", data: { navigable_list_target: "item", notifications_tray_target: "notification", - card_id: notification.card.id, timestamp: notification.created_at.to_i } do - concat( - link_to(notification, - class: [ "card card--notification", { "card--closed": notification.card.closed? }, { "unread": !notification.read? } ], - data: { turbo_frame: "_top", badge_target: "unread", action: "badge#update" }, - style: { "--card-color:": notification.card.color }, - &) - ) + card_id: notification.card.id, + timestamp: notification.created_at.to_i + } do + link_to(notification, + class: [ "card card--notification", { "card--closed": notification.card.closed? }, { "unread": !notification.read? } ], + data: { turbo_frame: "_top", badge_target: "unread", action: "badge#update" }, + style: { "--card-color:": notification.card.color }, + &) end end From 3625585a9dfbfc8279da58e4bb679acbb1c73154 Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Wed, 12 Nov 2025 11:15:25 -0600 Subject: [PATCH 5/7] Adjust unread indicator to include unread count --- app/assets/stylesheets/cards.css | 36 +++++++------------ app/assets/stylesheets/notifications.css | 19 +++------- app/assets/stylesheets/trays.css | 5 --- app/helpers/notifications_helper.rb | 3 +- .../notifications/_notification.html.erb | 1 - 5 files changed, 18 insertions(+), 46 deletions(-) diff --git a/app/assets/stylesheets/cards.css b/app/assets/stylesheets/cards.css index 2591332d9..19ab9740c 100644 --- a/app/assets/stylesheets/cards.css +++ b/app/assets/stylesheets/cards.css @@ -464,43 +464,31 @@ } .card__notification-unread-indicator { - --btn-background: var(--color-canvas); + --btn-background: var(--color-marker); --btn-border-color: var(--color-canvas); - --btn-icon-size: 0.75em; + --btn-color: var(--color-ink-inverted); + --btn-icon-size: 0.5em; --btn-padding: 0; - --btn-size: 1.25em; + --btn-size: 1.6em; + font-size: var(--text-xx-small); + font-weight: 600; + margin: 2px; position: relative; z-index: 1; - &:before, - .icon { - transition: opacity 150ms ease; - } - - /* The red dot */ - &:before { - aspect-ratio: 1; - background-color: var(--color-marker); - border-radius: 50%; - content: ""; - inline-size: 50%; - inset: 50% auto auto 50%; - position: absolute; - translate: -50% -50%; - } - - /* The X */ .icon { opacity: 0; + transition: opacity 150ms ease; } @media (hover: hover) { .card:hover & { - --btn-background: transparent; + --btn-background: var(--color-ink-lightest); + --btn-color: var(--color-ink); - &:before { opacity: 0; } - .icon { opacity: 1; } + .badge-count { opacity: 0; } + .icon { opacity: 1; } } } } diff --git a/app/assets/stylesheets/notifications.css b/app/assets/stylesheets/notifications.css index b50e35a2c..0cf434da8 100644 --- a/app/assets/stylesheets/notifications.css +++ b/app/assets/stylesheets/notifications.css @@ -29,10 +29,6 @@ column-gap: var(--inline-space-half); } - .card__notification-unread-indicator { - font-size: var(--text-small); - } - &:has(.card--notification) { .notifications-list__empty-message { display: none; @@ -40,13 +36,6 @@ } } - /* Grouped notifications - /* ------------------------------------------------------------------------ */ - - .notification--grouped { - border: 3px solid red; - } - /* Read items /* ------------------------------------------------------------------------ */ @@ -61,12 +50,14 @@ .card__notification-unread-indicator { --btn-background: transparent; + --btn-size: 1.8em; - &:before { - opacity: 0; - } + margin: 2px; .icon { + block-size: 1.7em; + color: var(--color-ink); + inline-size: 1.7em; opacity: 1; } } diff --git a/app/assets/stylesheets/trays.css b/app/assets/stylesheets/trays.css index 2352cb2b4..bb2538a7f 100644 --- a/app/assets/stylesheets/trays.css +++ b/app/assets/stylesheets/trays.css @@ -550,11 +550,6 @@ } } - .card__notification-unread-indicator { - --btn-icon-size: 1.25em; - --btn-size: 2em; - } - /* On mobile… */ @media (max-width: 799px) { /* …add a border */ diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index dda94684d..29348493a 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -43,7 +43,6 @@ module NotificationsHelper data: { action: "form#submit:stop badge#update:stop", form_target: "submit" }, form: { data: { controller: "form" } } do concat(icon_tag("unseen")) - concat(tag.span("Mark as unread", class: "for-screen-reader")) end else button_to notification_reading_path(notification), @@ -52,7 +51,7 @@ module NotificationsHelper data: { action: "form#submit:stop badge#update:stop", form_target: "submit" }, form: { data: { controller: "form" } } do concat(icon_tag("remove")) - concat(tag.span("Mark as read", class: "for-screen-reader")) + concat(tag.span("1", class: "badge-count", data: { group_count: "" })) end end end diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 49abafcb6..b945960b8 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -3,7 +3,6 @@
<%= render "notifications/notification/#{notification.source_type.underscore}/header", notification: notification %> -
From c0247671f7fe43780adfe0e96d818bc49a18e5c4 Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Wed, 12 Nov 2025 12:08:49 -0600 Subject: [PATCH 6/7] Fix regression that made index full-width --- app/assets/stylesheets/panels.css | 4 ++++ app/views/notifications/index.html.erb | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/panels.css b/app/assets/stylesheets/panels.css index a82dc1665..93e60b58e 100644 --- a/app/assets/stylesheets/panels.css +++ b/app/assets/stylesheets/panels.css @@ -15,6 +15,10 @@ } } + .panel--wide { + --panel-size: 60ch; + } + .panel--centered { --panel-border-size: 0; --panel-size: 100%; diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index 38ac87f52..2fa4e0078 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -13,7 +13,7 @@
-
+
<% if @unread.any? %>

New for you

@@ -30,7 +30,7 @@
-
+

Previously seen

From 8f4b6926f50d9046fb0ea6710e3a73a50c2c4f38 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 13 Nov 2025 09:41:45 +0100 Subject: [PATCH 7/7] Fiz test --- test/system/smoke_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/system/smoke_test.rb b/test/system/smoke_test.rb index d899aa450..92cb287fd 100644 --- a/test/system/smoke_test.rb +++ b/test/system/smoke_test.rb @@ -38,7 +38,7 @@ class SmokeTest < ApplicationSystemTestCase test "dismissing notifications" do sign_in_as(users(:david)) - notif = notifications(:logo_card_david_mention_by_jz) + notif = notifications(:logo_comment_david_mention_by_jz) assert_selector "div##{dom_id(notif)}"