From 86022a967cf1612bea0940888641f389b9063adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?luis=CE=B4=CE=BC?= Date: Wed, 12 Mar 2025 14:37:39 +0100 Subject: [PATCH] :sparkles: Replace overlapping bubbles with a bubble group (#6059) --- CHANGES.md | 3 +- .../workspace/get-comment-threads-unread.json | 58 ++++++++++ frontend/playwright/ui/pages/WorkspacePage.js | 25 ++++ .../ui/specs/workspace-comments.spec.js | 37 ++++++ .../src/app/main/data/workspace/comments.cljs | 82 +++++++++---- frontend/src/app/main/ui/comments.cljs | 109 +++++++++++++++--- frontend/src/app/main/ui/comments.scss | 11 ++ .../src/app/main/ui/dashboard/comments.cljs | 2 +- .../main/ui/workspace/viewport/comments.cljs | 18 ++- 9 files changed, 299 insertions(+), 46 deletions(-) create mode 100644 frontend/playwright/data/workspace/get-comment-threads-unread.json create mode 100644 frontend/playwright/ui/specs/workspace-comments.spec.js diff --git a/CHANGES.md b/CHANGES.md index 182695040..2c2c7675a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,7 +10,8 @@ ### :sparkles: New features -- "Mark All as Read" Functionality in Dashboard [Taiga #9235](https://tree.taiga.io/project/penpot/us/9235) +- [COMMENTS] "Mark All as Read" Functionality in Dashboard [Taiga #9235](https://tree.taiga.io/project/penpot/us/9235) +- [COMMENTS] Bubble Groups [Taiga #9236](https://tree.taiga.io/project/penpot/us/9236) - Change templates carrousel [Taiga #9803](https://tree.taiga.io/project/penpot/us/9803) ### :bug: Bugs fixed diff --git a/frontend/playwright/data/workspace/get-comment-threads-unread.json b/frontend/playwright/data/workspace/get-comment-threads-unread.json new file mode 100644 index 000000000..3f35d6aa2 --- /dev/null +++ b/frontend/playwright/data/workspace/get-comment-threads-unread.json @@ -0,0 +1,58 @@ +[ + { + "~:page-name":"Page 1", + "~:file-id":"~ud192fd06-a3e6-80d5-8004-7b7aaaea2a23", + "~:participants":{ + "~#set":[ + "~u0515a066-e303-8169-8004-73eb4018f4e0" + ] + }, + "~:content":"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Pellentesque lacus tellus, pretium id dapibus in, suscipit eu magna. Duis rhoncus, nisl quis accumsan euismod, dolor ipsum bibendum enim, et varius turpis erat ut purus.", + "~:count-unread-comments":1, + "~:count-comments":1, + "~:modified-at":"~m1718001240857", + "~:page-id":"~udd5cc0bb-91ff-81b9-8004-77df9cd3edb2", + "~:id": "~udd5cc0bb-91ff-81b9-8004-77df9cd3edb1", + "~:file-name":"New File 3", + "~:seqn":1, + "~:is-resolved":false, + "~:owner-id":"~u2e2da0fa-2d3e-81ec-8003-cb4453324510", + "~:position":{ + "~#point":{ + "~:x":120.0, + "~:y":120.0 + } + }, + "~:frame-id": "~uec508673-9e3b-80bf-8004-77dfa30a2b13", + "~:project-id": "~u0515a066-e303-8169-8004-73eb401b5d55", + "~:created-at":"~m1718001240857" + }, + { + "~:page-name":"Page 1", + "~:file-id":"~ud192fd06-a3e6-80d5-8004-7b7aaaea2a23", + "~:participants":{ + "~#set":[ + "~u0515a066-e303-8169-8004-73eb4018f4e0" + ] + }, + "~:content":"Duis lobortis ultricies lectus, in tristique tortor. Praesent mauris mi, finibus vel imperdiet quis, congue vel erat. Sed pharetra et ipsum at vestibulum.", + "~:count-unread-comments":0, + "~:count-comments":1, + "~:modified-at":"~m1718001247587", + "~:page-id":"~udd5cc0bb-91ff-81b9-8004-77df9cd3edb2", + "~:id":"~ud192fd06-a3e6-80d5-8004-7b7ac25ac93a", + "~:file-name":"New File 44", + "~:seqn":2, + "~:is-resolved":false, + "~:owner-id":"~u2e2da0fa-2d3e-81ec-8003-cb4453324510", + "~:position":{ + "~#point":{ + "~:x":160.0, + "~:y":160.0 + } + }, + "~:frame-id": "~uec508673-9e3b-80bf-8004-77dfa30a2b13", + "~:project-id":"~u343837a3-0d75-808a-8004-659df7b7873e", + "~:created-at":"~m1718001247587" + } +] diff --git a/frontend/playwright/ui/pages/WorkspacePage.js b/frontend/playwright/ui/pages/WorkspacePage.js index 6cc0057f6..96646a157 100644 --- a/frontend/playwright/ui/pages/WorkspacePage.js +++ b/frontend/playwright/ui/pages/WorkspacePage.js @@ -171,6 +171,25 @@ export class WorkspacePage extends BaseWebSocketPage { ); } + async setupFileWithComments() { + await this.mockRPC( + "get-comment-threads?file-id=*", + "workspace/get-comment-threads-unread.json", + ); + await this.mockRPC( + "get-file-fragment?file-id=*&fragment-id=*", + "viewer/get-file-fragment-single-board.json", + ); + await this.mockRPC( + "get-comments?thread-id=*", + "workspace/get-thread-comments.json", + ); + await this.mockRPC( + "update-comment-thread-status", + "workspace/update-comment-thread-status.json", + ); + } + async clickWithDragViewportAt(x, y, width, height) { await this.page.waitForTimeout(100); await this.viewport.hover({ position: { x, y } }); @@ -266,4 +285,10 @@ export class WorkspacePage extends BaseWebSocketPage { await this.tokenThemesSetsSidebar.getByText("Edit").click(clickOptions); await expect(this.tokenThemeUpdateCreateModal).toBeVisible(); } + + async showComments(clickOptions = {}) { + await this.page + .getByRole("button", { name: "Comments (C)" }) + .click(clickOptions); + } } diff --git a/frontend/playwright/ui/specs/workspace-comments.spec.js b/frontend/playwright/ui/specs/workspace-comments.spec.js new file mode 100644 index 000000000..89d5fc1f4 --- /dev/null +++ b/frontend/playwright/ui/specs/workspace-comments.spec.js @@ -0,0 +1,37 @@ +import { test, expect } from "@playwright/test"; +import { WorkspacePage } from "../pages/WorkspacePage"; + +test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); +}); + +test("Group bubbles when zooming out if they overlap", async ({ page }) => { + const workspacePage = new WorkspacePage(page); + await workspacePage.setupEmptyFile(); + + await workspacePage.setupFileWithComments(); + + await workspacePage.goToWorkspace(); + + await workspacePage.showComments(); + + await expect(page.getByTestId("floating-thread-bubble-1")).toBeVisible(); + await expect(page.getByTestId("floating-thread-bubble-2")).toBeVisible(); + await expect(page.getByTestId("floating-thread-bubble-1-2")).toBeHidden(); + + const zoom = page.getByTitle("Zoom"); + await zoom.click(); + + const zoomOut = page.getByTitle("Zoom out"); + await zoomOut.click(); + await zoomOut.click(); + await zoomOut.click(); + await zoomOut.click(); + + await expect(page.getByTestId("floating-thread-bubble-1")).toBeHidden(); + await expect(page.getByTestId("floating-thread-bubble-2")).toBeHidden(); + await expect(page.getByTestId("floating-thread-bubble-1-2")).toBeVisible(); + await expect(page.getByTestId("floating-thread-bubble-1-2")).toHaveClass( + /unread/, + ); +}); diff --git a/frontend/src/app/main/data/workspace/comments.cljs b/frontend/src/app/main/data/workspace/comments.cljs index 36fe3c835..97bc7747e 100644 --- a/frontend/src/app/main/data/workspace/comments.cljs +++ b/frontend/src/app/main/data/workspace/comments.cljs @@ -20,6 +20,7 @@ [app.main.data.workspace.drawing :as dwd] [app.main.data.workspace.edition :as dwe] [app.main.data.workspace.selection :as dws] + [app.main.data.workspace.zoom :as dwz] [app.main.repo :as rp] [app.main.router :as rt] [app.main.streams :as ms] @@ -102,26 +103,6 @@ ny (- (:y position) nh)] (update local :vbox assoc :x nx :y ny))))))) -(defn navigate - [thread] - (dm/assert! - "expected valid comment thread" - (dcmt/check-comment-thread! thread)) - (ptk/reify ::open-comment-thread - ptk/WatchEvent - (watch [_ _ stream] - (rx/merge - (rx/of (dcm/go-to-workspace :file-id (:file-id thread) - :page-id (:page-id thread))) - - (->> stream - (rx/filter (ptk/type? ::dcmt/comment-threads-fetched)) - (rx/take 1) - (rx/mapcat #(rx/of (center-to-comment-thread thread) - (dwd/select-for-drawing :comments) - (with-meta (dcmt/open-thread thread) - {::ev/origin "workspace"})))))))) - (defn update-comment-thread-position ([thread [new-x new-y]] (update-comment-thread-position thread [new-x new-y] nil)) @@ -192,6 +173,66 @@ (map build-move-event) (rx/from)))))) +(defn overlap-bubbles? + "Detect if two bubbles overlap" + [zoom thread-1 thread-2] + (let [distance (gpt/distance (:position thread-1) (:position thread-2)) + distance-zoom (* distance zoom) + distance-overlap 32] + (< distance-zoom distance-overlap))) + +(defn- calculate-zoom-scale-to-ungroup-current-bubble + "Calculate the minimum zoom scale needed to keep the current bubble ungrouped from the rest" + [zoom thread threads] + (let [threads-rest (filterv #(not= (:id %) (:id thread)) threads) + zoom-scale-step 1.75] + (if (some #(overlap-bubbles? zoom thread %) threads-rest) + (calculate-zoom-scale-to-ungroup-current-bubble (* zoom zoom-scale-step) thread threads) + zoom))) + +(defn set-zoom-to-separate-grouped-bubbles + [thread] + (dm/assert! + "zoom-to-separate-bubbles" + (dcmt/check-comment-thread! thread)) + (ptk/reify ::set-zoom-to-separate-grouped-bubbles + ptk/WatchEvent + (watch [_ state _] + (let [local (:workspace-local state) + zoom (:zoom local) + page-id (:page-id thread) + + threads-map (:comment-threads state) + threads-all (vals threads-map) + threads (filterv #(= (:page-id %) page-id) threads-all) + + updated-zoom (calculate-zoom-scale-to-ungroup-current-bubble zoom thread threads) + scale-zoom (/ updated-zoom zoom)] + + (rx/of (dwz/set-zoom scale-zoom)))))) + +(defn navigate-to-comment-from-dashboard + [thread] + (dm/assert! + "expected valid comment thread" + (dcmt/check-comment-thread! thread)) + (ptk/reify ::navigate-to-comment-from-dashboard + ptk/WatchEvent + (watch [_ _ stream] + (rx/merge + (rx/of (dcm/go-to-workspace :file-id (:file-id thread) + :page-id (:page-id thread))) + + (->> stream + (rx/filter (ptk/type? :app.main.data.workspace/workspace-initialized)) + (rx/observe-on :async) + (rx/take 1) + (rx/mapcat #(rx/of (dwd/select-for-drawing :comments) + (set-zoom-to-separate-grouped-bubbles thread) + (center-to-comment-thread thread) + (with-meta (dcmt/open-thread thread) + {::ev/origin "workspace"})))))))) + (defn navigate-to-comment [thread] (ptk/reify ::navigate-to-comment @@ -208,6 +249,7 @@ (rx/empty)) (->> (rx/of (dwd/select-for-drawing :comments) + (set-zoom-to-separate-grouped-bubbles thread) (center-to-comment-thread thread) (with-meta (dcmt/open-thread thread) {::ev/origin "workspace"})) (rx/observe-on :async)))))) diff --git a/frontend/src/app/main/ui/comments.cljs b/frontend/src/app/main/ui/comments.cljs index 1b6d99244..bb8f0fc2f 100644 --- a/frontend/src/app/main/ui/comments.cljs +++ b/frontend/src/app/main/ui/comments.cljs @@ -17,6 +17,7 @@ [app.main.data.comments :as dcm] [app.main.data.modal :as modal] [app.main.data.workspace.comments :as dwcm] + [app.main.data.workspace.zoom :as dwz] [app.main.refs :as refs] [app.main.store :as st] [app.main.ui.components.dropdown :refer [dropdown]] @@ -589,22 +590,25 @@ (def ^:private schema:comment-avatar [:map [:class {:optional true} :string] - [:image :string] + [:image {:optional true} :string] [:variant {:optional true} [:maybe [:enum "read" "unread" "solved"]]]]) (mf/defc comment-avatar* {::mf/schema schema:comment-avatar} - [{:keys [image variant class] :rest props}] + [{:keys [image variant class children] :rest props}] (let [variant (or variant "read") - class (dm/str class " " (stl/css-case :avatar true - :avatar-read (= variant "read") - :avatar-unread (= variant "unread") - :avatar-solved (= variant "solved"))) - props (mf/spread-props props {:class class})] + class (dm/str class " " (stl/css-case :avatar true + :avatar-read (= variant "read") + :avatar-unread (= variant "unread") + :avatar-solved (= variant "solved"))) + props (mf/spread-props props {:class class})] + [:> :div props - [:img {:src image - :class (stl/css :avatar-image)}] + (if image + [:img {:src image + :class (stl/css :avatar-image)}] + [:div {:class (stl/css :avatar-text)} children]) [:div {:class (stl/css-case :avatar-mask true :avatar-darken (= variant "solved"))}]])) @@ -1072,22 +1076,83 @@ [:> mentions-panel*]])])) +(defn group-bubbles + "Group bubbles in different vectors by proximity" + ([zoom circles] + (group-bubbles zoom circles [] [])) + + ([zoom circles visited groups] + (if (empty? circles) + groups + (let [current (first circles) + remaining (rest circles) + overlapping-group (some (fn [group] + (when (some (partial dwcm/overlap-bubbles? zoom current) group) group)) + groups)] + (if overlapping-group + (group-bubbles zoom remaining visited (map (fn [group] + (if (= group overlapping-group) + (cons current group) + group)) + groups)) + (group-bubbles zoom remaining visited (cons [current] groups))))))) + +(defn- calculate-zoom-scale-to-ungroup-bubbles + "Calculate the minimum zoom scale needed for a group of bubbles to avoid overlap among them" + [zoom threads] + (let [num-threads (count threads) + num-grouped-threads (count (group-bubbles zoom threads)) + zoom-scale-step 1.75] + (if (= num-threads num-grouped-threads) + zoom + (calculate-zoom-scale-to-ungroup-bubbles (* zoom zoom-scale-step) threads)))) + +(mf/defc comment-floating-group* + {::mf/wrap [mf/memo]} + [{:keys [thread-group zoom position-modifier]}] + (let [positions (mapv :position thread-group) + + position (gpt/center-points positions) + position (cond-> position + (some? position-modifier) + (gpt/transform position-modifier)) + pos-x (* (:x position) zoom) + pos-y (* (:y position) zoom) + + unread? (some #(pos? (:count-unread-comments %)) thread-group) + num-threads (str (count thread-group)) + + test-id (str/join "-" (map :seqn (sort-by :seqn thread-group))) + + on-click + (mf/use-fn + (mf/deps thread-group position) + (fn [] + (let [updated-zoom (calculate-zoom-scale-to-ungroup-bubbles zoom thread-group) + scale-zoom (/ updated-zoom zoom)] + (st/emit! (dwz/set-zoom position scale-zoom)))))] + + [:div {:style {:top (dm/str pos-y "px") + :left (dm/str pos-x "px")} + :on-click on-click + :class (stl/css :floating-preview-wrapper :floating-preview-bubble)} + [:> comment-avatar* + {:class (stl/css :avatar-lg) + :variant (if unread? "unread" "read") + :data-testid (dm/str "floating-thread-bubble-" test-id)} + num-threads]])) + (mf/defc comment-floating-bubble* {::mf/wrap [mf/memo]} [{:keys [thread zoom is-open on-click origin position-modifier]}] (let [owner (mf/with-memo [thread] (dcm/get-owner thread)) - base-pos (cond-> (:position thread) + + position (:position thread) + position (cond-> position (some? position-modifier) (gpt/transform position-modifier)) - drag? (mf/use-ref nil) - was-open? (mf/use-ref nil) - - dragging-ref (mf/use-ref false) - start-ref (mf/use-ref nil) - - position (:position thread) frame-id (:frame-id thread) state (mf/use-state @@ -1097,8 +1162,14 @@ :new-position-y nil :new-frame-id frame-id})) - pos-x (floor (* (or (:new-position-x @state) (:x base-pos)) zoom)) - pos-y (floor (* (or (:new-position-y @state) (:y base-pos)) zoom)) + pos-x (floor (* (or (:new-position-x @state) (:x position)) zoom)) + pos-y (floor (* (or (:new-position-y @state) (:y position)) zoom)) + + drag? (mf/use-ref nil) + was-open? (mf/use-ref nil) + + dragging-ref (mf/use-ref false) + start-ref (mf/use-ref nil) on-pointer-down (mf/use-fn diff --git a/frontend/src/app/main/ui/comments.scss b/frontend/src/app/main/ui/comments.scss index 399a6ac46..4e4eb4345 100644 --- a/frontend/src/app/main/ui/comments.scss +++ b/frontend/src/app/main/ui/comments.scss @@ -91,6 +91,17 @@ border-radius: $br-circle; } +.avatar-text { + border-radius: $br-circle; + height: 100%; + width: 100%; + display: flex; + align-items: center; + justify-content: center; + font-size: $fs-12; + background-color: var(--color-background-quaternary); +} + .avatar-mask { border-radius: $br-circle; position: absolute; diff --git a/frontend/src/app/main/ui/dashboard/comments.cljs b/frontend/src/app/main/ui/dashboard/comments.cljs index 02384a66b..ab9d800f6 100644 --- a/frontend/src/app/main/ui/dashboard/comments.cljs +++ b/frontend/src/app/main/ui/dashboard/comments.cljs @@ -62,7 +62,7 @@ on-navigate (mf/use-callback (fn [thread] - (st/emit! (-> (dwcm/navigate thread) + (st/emit! (-> (dwcm/navigate-to-comment-from-dashboard thread) (with-meta {::ev/origin "dashboard"}))))) on-read-all diff --git a/frontend/src/app/main/ui/workspace/viewport/comments.cljs b/frontend/src/app/main/ui/workspace/viewport/comments.cljs index 30a2d6dae..2b67b9b6a 100644 --- a/frontend/src/app/main/ui/workspace/viewport/comments.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/comments.cljs @@ -16,6 +16,7 @@ [rumext.v2 :as mf])) (mf/defc comments-layer* + {::mf/wrap [mf/memo]} [{:keys [vbox vport zoom file-id page-id]}] (let [vbox-x (dm/get-prop vbox :x) vbox-y (dm/get-prop vbox :y) @@ -59,11 +60,18 @@ :height (dm/str vport-h "px")}} [:div {:class (stl/css :threads) :style {:transform (dm/fmt "translate(%px, %px)" pos-x pos-y)}} - (for [item threads] - [:> cmt/comment-floating-bubble* {:thread item - :zoom zoom - :is-open (= (:id item) (:open local)) - :key (:seqn item)}]) + + (for [thread-group (cmt/group-bubbles zoom threads)] + (let [group? (> (count thread-group) 1) + thread (first thread-group)] + (if group? + [:> cmt/comment-floating-group* {:thread-group thread-group + :zoom zoom + :key (:seqn thread)}] + [:> cmt/comment-floating-bubble* {:thread thread + :zoom zoom + :is-open (= (:id thread) (:open local)) + :key (:seqn thread)}]))) (when-let [id (:open local)] (when-let [thread (get threads-map id)]