From 9ae262b0ff9e11ad9c033ae29b158f8c991f6ebb Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Wed, 28 Oct 2026 08:45:58 +0100 Subject: [PATCH 1/4] :sparkles: Show added and deleted layers in the board that holds them The compare dialog drew nothing for a deleted layer when the branch was open, and nothing for an added one when main was: the diff named the layer by id and label only, and the open file lacks it. The detail pane was left with the layer's name and nothing else. Added and deleted shape entries now carry the whole shape, as the document that holds it has it, under `:shape`. The key names no side, so it reads the same in both directions. The previews use it. A layer added or deleted inside a board is shown in that board on both sides, outlined in green or red on the side that holds it, so the change reads as what the board gains or loses. The facts of the detail pane (where it was, type, size) read it as well. A board that moved or resized now draws at its own size on the side the open file lacks: the diff leaves out `:selrect` and `:points`, so they are rebuilt from position and size instead of kept from the open side. --- common/src/app/common/files/branch_merge.cljc | 15 +++- .../common_tests/files_branch_merge_test.cljc | 22 +++++ .../sidebar/branch_review/compare.cljs | 41 ++++++--- .../sidebar/branch_review/facts.cljs | 15 +++- .../sidebar/branch_review/previews.cljs | 85 ++++++++++++++----- .../sidebar/branch_review/previews.scss | 15 ++++ frontend/test/frontend_tests/runner.cljs | 2 + .../ui/branch_review_previews_test.cljs | 59 +++++++++++++ 8 files changed, 218 insertions(+), 36 deletions(-) create mode 100644 frontend/test/frontend_tests/ui/branch_review_previews_test.cljs diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index 262d3785aa..fc0d34ffe1 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -839,6 +839,19 @@ :base (get bo (:id e)) :main (get to (:id e)) :branch (get oo (:id e)))) + ;; an added or deleted shape is missing from + ;; one of the two documents, and the compare + ;; panel may have that one open: the entry + ;; carries the FULL shape as the side that + ;; holds it has it, so the panel can draw it + ;; and say what it is. `:shape` names no side, + ;; so `doc-sides` leaves it alone + with-shape (fn [e] + (case (:status e) + :added (assoc e :shape (get oo (:id e))) + :deleted (assoc e :shape (or (get bo (:id e)) + (get to (:id e)))) + e)) passed (tree-policy-pass res bo to oo sbo sto reorders pid modes)] {:changes (mapv (fn [e] ;; a `:refuse` entry names the @@ -847,7 +860,7 @@ ;; display (if (= :unsupported (:status e)) e - (enrich e))) + (-> e enrich with-shape))) (:changes passed)) :conflicts (mapv (comp rehydrate enrich) (:conflicts passed))})) common)] diff --git a/common/test/common_tests/files_branch_merge_test.cljc b/common/test/common_tests/files_branch_merge_test.cljc index 633daaf2a5..cbb430f593 100644 --- a/common/test/common_tests/files_branch_merge_test.cljc +++ b/common/test/common_tests/files_branch_merge_test.cljc @@ -154,6 +154,28 @@ (t/is (= :deleted (:status c))) (t/is (= :s2 (:id c))))) +(t/deftest added-and-deleted-carry-the-shape + ;; the open file lacks the shape on one side, so the entry carries it + ;; whole, as the document that holds it has it, in both directions + (let [base (mkdata {:s1 {:id :s1 :name "A" :selrect {:x 1}} + :s2 {:id :s2 :name "B" :selrect {:x 2}}}) + branch (mkdata {:s1 {:id :s1 :name "A" :selrect {:x 1}} + :s3 {:id :s3 :name "C" :selrect {:x 3}}}) + merge (into {} (map (juxt :id identity)) + (:changes (bm/compute-merge base base branch :branch->main))) + update (into {} (map (juxt :id identity)) + (:changes (bm/compute-merge base branch base :main->branch)))] + (t/is (= {:id :s2 :name "B" :selrect {:x 2}} (get-in merge [:s2 :shape]))) + (t/is (= {:id :s3 :name "C" :selrect {:x 3}} (get-in merge [:s3 :shape]))) + (t/is (= {:id :s2 :name "B" :selrect {:x 2}} (get-in update [:s2 :shape]))) + (t/is (= {:id :s3 :name "C" :selrect {:x 3}} (get-in update [:s3 :shape]))))) + +(t/deftest modified-carries-no-shape + (let [base (mkdata {:s1 {:id :s1 :name "A" :fill "red"}}) + branch (mkdata {:s1 {:id :s1 :name "A" :fill "blue"}}) + r (bm/compute-merge base base branch :branch->main)] + (t/is (not (contains? (first (:changes r)) :shape))))) + (t/deftest fast-forward-main-untouched ;; main == base; branch changed -> all branch changes apply cleanly (let [base (mkdata {:s1 {:id :s1 :name "A" :fill "red"}}) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs index 6228fc7d23..3e6aadf874 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs @@ -226,7 +226,7 @@ "MAIN and BRANCH, side by side, each with what it holds for the change." {::mf/private true} [{:keys [kind main-value branch-value main-objects branch-objects - main-detail branch-detail main-missing branch-missing large]}] + main-detail branch-detail main-missing branch-missing large highlight]}] [:div {:class (stl/css :cards)} [:> preview-card* {:label (tr "workspace.branches.review.side.main") :side "main" @@ -235,7 +235,8 @@ [:> brp/entity-preview* {:kind kind :value main-value :objects main-objects - :missing main-missing}]] + :missing main-missing + :highlight highlight}]] [:> preview-card* {:label (tr "workspace.branches.review.side.branch") :side "branch" :detail branch-detail @@ -243,7 +244,8 @@ [:> brp/entity-preview* {:kind kind :value branch-value :objects branch-objects - :missing branch-missing}]]]) + :missing branch-missing + :highlight highlight}]]]) (mf/defc view-modes* "Side by side or overlay for the previews of a change, as icon toggles, @@ -282,12 +284,23 @@ (when objects (brp/side-objects objects page-items :main open-side))) branch-objs (mf/with-memo [objects page-items open-side] (when objects (brp/side-objects objects page-items :branch open-side))) - main-v (if (= :shape kind) - (or (get main-objs (:id item)) (get item :main)) - (brp/side-entity item :main open-side (library-entity data item))) - branch-v (if (= :shape kind) - (or (get branch-objs (:id item)) (get item :branch)) - (brp/side-entity item :branch open-side (library-entity data item))) + ;; an added or deleted layer inside a board is shown in it: the + ;; board on both sides, the layer outlined on the side that holds + ;; it, so the change reads as what the board gains or loses + stage (when (and (= :shape kind) (contains? #{:added :deleted} (:status item))) + (let [holder (if (contains? main-objs (:id item)) main-objs branch-objs) + fid (brp/context-frame-id holder (:id item))] + (when (and (some? fid) (contains? main-objs fid) (contains? branch-objs fid)) + fid))) + highlight (when stage {:id (:id item) :status (:status item)}) + main-v (cond + stage (get main-objs stage) + (= :shape kind) (or (get main-objs (:id item)) (get item :main)) + :else (brp/side-entity item :main open-side (library-entity data item))) + branch-v (cond + stage (get branch-objs stage) + (= :shape kind) (or (get branch-objs (:id item)) (get item :branch)) + :else (brp/side-entity item :branch open-side (library-entity data item))) main-detail (some-> (:main-at diff-meta) ct/inst ct/timeago) branch-detail (some-> (:branch-at diff-meta) ct/inst ct/timeago)] (when (or main-v branch-v) @@ -300,12 +313,15 @@ [:div {:class (stl/css :overlay-stage)} (when main-v [:div {:class (stl/css :overlay-layer)} - [:> brp/entity-preview* {:kind kind :value main-v :objects main-objs}]]) + [:> brp/entity-preview* {:kind kind :value main-v :objects main-objs + :highlight highlight}]]) (when branch-v [:div {:class (stl/css :overlay-layer :is-top)} - [:> brp/entity-preview* {:kind kind :value branch-v :objects branch-objs}]])]] + [:> brp/entity-preview* {:kind kind :value branch-v :objects branch-objs + :highlight highlight}]])]] [:> side-cards* {:kind kind :large large + :highlight highlight :main-value main-v :branch-value branch-v :main-objects main-objs @@ -402,7 +418,8 @@ :shape (when (contains? #{:added :deleted} (:status item)) - [:> brf/shape-facts* {:item item :data data :incoming incoming}]) + [:> brf/shape-facts* {:item item :items items :data data + :open-side open-side :incoming incoming}]) (:component :color :typography :token) [:> brf/usage-facts* {:item item :data data}] diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs index d2f17acb21..571afd1f50 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs @@ -8,7 +8,7 @@ "What the detail pane says about an entry that has no before and after: a new or deleted page or layer, and the library entries other layers depend on. It answers what the entry is, where it lands and what it - affects, from the open file." + affects, from the open file and, for a layer it lacks, from the diff." (:require-macros [app.main.style :as stl]) (:require [app.common.data.macros :as dm] @@ -137,8 +137,17 @@ (mf/defc shape-facts* "A new or deleted layer: where it lands (or was), what it is and, for a deleted main component, the copies it leaves behind." - [{:keys [item data incoming]}] - (let [objects (get-in data [:pages-index (:page-id item) :objects]) + [{:keys [item items data open-side incoming]}] + (let [open-objs (get-in data [:pages-index (:page-id item) :objects]) + ;; a layer the open file lacks is read on the other side, where + ;; the changes of `items` put it and its deleted parents back + objects (mf/with-memo [open-objs items item open-side] + (if (or (contains? open-objs (:id item)) (nil? open-objs)) + open-objs + (brp/side-objects open-objs + (filterv #(= (:page-id item) (:page-id %)) items) + (if (= open-side :main) :branch :main) + open-side))) shape (get objects (:id item)) page (get-in data [:pages-index (:page-id item) :name]) deleted? (= :deleted (:status item)) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs index 7b6eb7e431..9edbc319a1 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs @@ -6,12 +6,13 @@ (ns app.main.ui.workspace.sidebar.branch-review.previews "Visual previews of the two sides of a change. The diff carries whole - entities only for conflicts; for a clean change the open file holds one - side and the changed attrs give the other, which is enough to draw both - the layer and the board it sits in." + entities for conflicts and for added or deleted shapes; for a modified + entity the open file holds one side and the changed attrs give the + other, which is enough to draw both the layer and the board it sits in." (:require-macros [app.main.style :as stl]) (:require [app.common.data.macros :as dm] + [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.main.render :as render] [app.main.ui.ds.foundations.assets.icon :as i] @@ -28,6 +29,11 @@ icon instead." #{:rect :circle :frame :path :bool :text :group}) +(def ^:private box-shape-types + "Shape types whose geometry is their position and size; a path or a + boolean takes it from its content, a group from its children." + #{:rect :circle :frame :text :image :svg-raw}) + (defn open-side "Which document the open file is for `branch`: `:branch` when it is the branch file, `:main` otherwise." @@ -41,7 +47,8 @@ both; a clean change carries the attrs that differ, so the side the open file does not hold is the open entity with those attrs swapped in. An entry that does not exist on a side (added on the other one, or - deleted) has no entity there." + deleted) has no entity there. An added or deleted shape the open file + lacks comes whole in the entry's `:shape`." [item side open-side open-entity] (cond (= :conflict (:status item)) @@ -51,7 +58,7 @@ open-entity (nil? open-entity) - nil + (:shape item) (and (= :added (:status item)) (= side :main)) nil @@ -60,18 +67,27 @@ nil :else - (reduce-kv (fn [entity attr values] - (let [v (get values side)] - (if (nil? v) - (dissoc entity attr) - (assoc entity attr v)))) - open-entity - (:changed-attrs item)))) + (let [attrs (:changed-attrs item) + entity (reduce-kv (fn [entity attr values] + (let [v (get values side)] + (if (nil? v) + (dissoc entity attr) + (assoc entity attr v)))) + open-entity + attrs)] + ;; the diff leaves out `:selrect` and `:points` (they follow from + ;; position and size), so a box that moved or resized gets them + ;; again from its own, or it would draw with the open side's + (if (and (contains? box-shape-types (:type entity)) + (some #(contains? attrs %) [:x :y :width :height])) + (cts/setup-rect (dissoc entity :selrect :points)) + entity)))) (defn side-objects "The page `objects` as they stand on `side`, with every shape change of `items` that lives on this page put on that side. A shape missing on - that side is taken out of its parent, so boards draw without it." + that side is taken out of its parent, so boards draw without it, and a + shape only that side holds is put into its parent, so boards draw it." [objects items side open-side] (reduce (fn [objects item] @@ -80,7 +96,12 @@ (let [id (:id item) entity (side-entity item side open-side (get objects id))] (if (some? entity) - (assoc objects id entity) + (let [parent-id (:parent-id entity)] + (cond-> (assoc objects id entity) + (and (contains? objects parent-id) + (not (contains? objects id)) + (not (some #(= id %) (get-in objects [parent-id :shapes])))) + (update-in [parent-id :shapes] (fnil conj []) id))) (let [parent-id (:parent-id (get objects id))] (cond-> (dissoc objects id) (contains? objects parent-id) @@ -100,23 +121,47 @@ (let [sr (:selrect shape)] {:x (:x sr 0) :y (:y sr 0) :width (:width sr 1) :height (:height sr 1)})))) +(defn context-frame-id + "The nearest board that holds shape `id` in `objects`, the page root + left out: the stage an added or deleted layer is shown on." + [objects id] + (loop [parent-id (:parent-id (get objects id)) + depth 0] + (let [parent (get objects parent-id)] + (cond + (or (nil? parent) (= parent-id uuid/zero) (>= depth 64)) nil + (= :frame (:type parent)) parent-id + :else (recur (:parent-id parent) (inc depth)))))) + (mf/defc shape-preview* "Renders `shape` to a fitted SVG with the real shape renderers. `objects` - supplies its children (a board or group draws them)." + supplies its children (a board or group draws them). `highlight` + (`{:id :status}`) outlines the layer an added or deleted change is + about, when `objects` holds it." {::mf/private true} - [{:keys [shape objects]}] + [{:keys [shape objects highlight]}] (let [{:keys [x y width height]} (shape-bounds shape) width (max 1 width) height (max 1 height) objects (mf/with-memo [shape objects] (assoc (or objects {}) (:id shape) shape)) + marked (some->> (:id highlight) (get objects) shape-bounds) wrapper (mf/with-memo [objects] (render/shape-wrapper-factory objects))] [:svg {:class (stl/css :preview-svg) :viewBox (dm/str x " " y " " width " " height) :preserveAspectRatio "xMidYMid meet" :xmlns "http://www.w3.org/2000/svg" :fill "none"} - [:> wrapper {:shape shape}]])) + [:> wrapper {:shape shape}] + (when marked + [:rect {:class (stl/css-case :preview-highlight true + :is-added (= :added (:status highlight)) + :is-deleted (= :deleted (:status highlight))) + :x (:x marked) + :y (:y marked) + :width (max 1 (:width marked)) + :height (max 1 (:height marked)) + :vector-effect "non-scaling-stroke"}])])) (mf/defc page-preview* "The whole page `objects` drawn to fit: every top-level layer, framed @@ -192,8 +237,8 @@ shapes render to SVG, library colors to a swatch, typographies to a text sample; everything else shows a type icon. A side with nothing to draw says why (`missing`: `{:reason :side :open-side}`, see - `missing-reason`)." - [{:keys [kind value objects missing]}] + `missing-reason`). A shape `highlight` is passed on to `shape-preview*`." + [{:keys [kind value objects missing highlight]}] (cond (nil? value) [:> missing-preview* {:reason (:reason missing :unavailable) @@ -202,7 +247,7 @@ (= kind :shape) (if (contains? renderable-shape-types (:type value)) - [:> shape-preview* {:shape value :objects objects}] + [:> shape-preview* {:shape value :objects objects :highlight highlight}] [:div {:class (stl/css :preview-empty)} [:> i/icon* {:icon-id (brc/shape-icon-id {:shape-type (:type value)})}]]) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss index a90fd6b733..7130249a5b 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss @@ -13,6 +13,21 @@ block-size: 100%; } +// the outline of the layer an added or deleted change is about, drawn +// over the board that holds it; the stroke keeps its width at any zoom +.preview-highlight { + --preview-highlight-color: var(--color-accent-success); + + fill: color-mix(in srgb, var(--preview-highlight-color) 12%, transparent); + stroke: var(--preview-highlight-color); + stroke-width: 2; + stroke-dasharray: 4 3; + + &.is-deleted { + --preview-highlight-color: var(--color-accent-error); + } +} + .preview-swatch { inline-size: 100%; block-size: 100%; diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 77678bb47c..f7d6ad59cb 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -111,6 +111,7 @@ [frontend-tests.tokens.token-errors-test] [frontend-tests.tokens.workspace-tokens-remap-test] [frontend-tests.ui.branch-review-common-test] + [frontend-tests.ui.branch-review-previews-test] [frontend-tests.ui.branches-compare-labels-test] [frontend-tests.ui.check-updates-test] [frontend-tests.ui.color-token-position-test] @@ -270,6 +271,7 @@ 'frontend-tests.tokens.logic.tokens-status-test 'frontend-tests.tokens.workspace-tokens-remap-test 'frontend-tests.ui.branch-review-common-test + 'frontend-tests.ui.branch-review-previews-test 'frontend-tests.ui.branches-compare-labels-test 'frontend-tests.ui.check-updates-test 'frontend-tests.ui.color-token-position-test diff --git a/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs b/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs new file mode 100644 index 0000000000..0f99c7f85e --- /dev/null +++ b/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs @@ -0,0 +1,59 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.ui.branch-review-previews-test + "How the compare previews rebuild the side the open file lacks: a + deleted layer comes back from the diff, inside the board it was in, + and a resized board draws at its own size." + (:require + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.main.ui.workspace.sidebar.branch-review.previews :as brp] + [cljs.test :as t :include-macros true])) + +(def ^:private board-id (uuid/next)) +(def ^:private icon-id (uuid/next)) +(def ^:private path-id (uuid/next)) + +(def ^:private branch-objects + "The branch, open: the button lost its icon and shrank." + {uuid/zero {:id uuid/zero :type :frame :shapes [board-id]} + board-id (cts/setup-rect {:id board-id :type :frame :name "Button" + :parent-id uuid/zero :x 0 :y 0 :width 131 :height 48 + :shapes []})}) + +(def ^:private icon + {:id icon-id :type :group :name "icon" :parent-id board-id + :x 119 :y 12 :width 24 :height 24 :shapes [path-id]}) + +(def ^:private items + [{:kind :shape :status :deleted :id path-id + :shape {:id path-id :type :path :name "path" :parent-id icon-id}} + {:kind :shape :status :deleted :id icon-id :shape icon} + {:kind :shape :status :modified :id board-id + :changed-attrs {:width {:main 163 :branch 131}}}]) + +(t/deftest a-deleted-layer-comes-back-on-the-side-that-has-it + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= icon (get main icon-id))) + (t/is (contains? main path-id)) + (t/is (= [icon-id] (get-in main [board-id :shapes]))))) + +(t/deftest the-side-that-deleted-it-stays-without-it + (let [branch (brp/side-objects branch-objects items :branch :branch)] + (t/is (not (contains? branch icon-id))) + (t/is (= [] (get-in branch [board-id :shapes]))))) + +(t/deftest a-resized-board-draws-at-its-own-size + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= 163 (get-in main [board-id :selrect :width]))) + (t/is (= 131 (get-in branch-objects [board-id :selrect :width]))))) + +(t/deftest a-layer-is-staged-in-the-nearest-board + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= board-id (brp/context-frame-id main path-id))) + (t/is (= board-id (brp/context-frame-id main icon-id))) + (t/is (nil? (brp/context-frame-id main board-id))))) From 876ab29ba991718c376b3d2836679bf1dd63e982 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Wed, 28 Oct 2026 08:52:19 +0100 Subject: [PATCH 2/4] :sparkles: Fold the layers deleted with their container into one row Deleting a group listed the group and each of its layers as rows of their own, so one deletion read as four changes. Each row could be left out alone too, which allowed a merge to keep a group while deleting what it holds. A layer added or deleted with its container now folds under the outermost container added or deleted with it. The row says how many layers it holds and its checkbox takes them along, so they travel or stay together. The detail pane lists them as its contents. The filters, the footer, the merge button and the merge confirmation count the rows the list shows, so they agree with one another. --- .../sidebar/branch_review/common.cljs | 33 ++++++++ .../sidebar/branch_review/compare.cljs | 79 ++++++++++++++----- .../sidebar/branch_review/facts.cljs | 11 ++- .../main/ui/workspace/sidebar/branches.cljs | 12 ++- .../ui/branch_review_common_test.cljs | 26 ++++++ 5 files changed, 136 insertions(+), 25 deletions(-) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs index 7d5d302f5a..cf978891c0 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs @@ -383,6 +383,39 @@ (or frame-id (:parent-id shape)))) (inc depth))))))) +(defn- cascade-entry? + "An added or deleted layer that carries its shape: one that can take + the layers added or deleted with it." + [item] + (and (= :shape (:kind item)) + (contains? #{:added :deleted} (:status item)) + (some? (:shape item)))) + +(defn fold-cascades + "The added or deleted layers that only follow a container added or + deleted with them, folded under the outermost one: deleting a group + deletes its layers too, and listing each one apart would repeat the + same change. Takes `[index item]` pairs and returns a map from the + index of each outermost entry to the `[index item]` pairs it holds." + [indexed] + (let [by-id (into {} + (keep (fn [[idx item]] + (when (cascade-entry? item) + [[(:page-id item) (:id item)] [idx item]]))) + indexed) + root-of (fn [item] + (loop [cur item root nil depth 0] + (let [[pidx parent] (get by-id [(:page-id item) (get-in cur [:shape :parent-id])])] + (if (and parent (= (:status parent) (:status item)) (< depth 64)) + (recur parent pidx (inc depth)) + root))))] + (reduce (fn [acc [_ item :as row]] + (if-let [root (when (cascade-entry? item) (root-of item))] + (update acc root (fnil conj []) row) + acc)) + {} + indexed))) + (defn group-by-place "Group the clean changes (`[index item]` pairs) by page, and inside a page by the top-level board that holds them, reading names from the diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs index 3e6aadf874..0759626733 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs @@ -101,11 +101,31 @@ :else nil))) +(defn- row-keys + "The change keys a row stands for: its own and those of the layers + folded under it (`brc/fold-cascades`), which travel or stay with it." + [item inner] + (into #{(:key item)} (map (comp :key second)) inner)) + +(defn- remove-folded + "`rows` (`[index item]` pairs) without the layers folded under another + row (`brc/fold-cascades`)." + [rows cascades] + (let [folded (into #{} (comp (mapcat val) (map first)) cascades)] + (filterv (fn [[idx _]] (not (contains? folded idx))) rows))) + (mf/defc item-row* + "One change. `inner` are the layers added or deleted with it, folded + under it: the row counts them and its checkbox takes them along." {::mf/private true} - [{:keys [index item selected excluded reviewed on-select on-toggle]}] + [{:keys [index item inner selected excluded reviewed on-select on-toggle]}] (let [selectable? (selectable-item? item) change-key (:key item) + toggle-keys (row-keys item inner) + subtitle (cond-> (brc/change-summary item) + (seq inner) + (dm/str " - " (tr "workspace.branches.review.facts.layers" + (i18n/c (count inner))))) on-click (mf/use-fn (mf/deps index on-select) #(on-select {:kind :item :index index})) start (when selectable? (mf/html @@ -114,9 +134,9 @@ :on-click dom-stop} [:> checkbox* {:id (dm/str "change-" change-key) :checked (not excluded) - :on-change #(on-toggle #{change-key})}]]))] + :on-change #(on-toggle toggle-keys)}]]))] [:> change-row* {:title (or (:label item) "") - :subtitle (brc/change-summary item) + :subtitle subtitle :icon (brc/entry-icon item) :tone (if (and excluded selectable?) "neutral" (brc/status-tone (:status item))) :is-selected (= selected {:kind :item :index index}) @@ -154,7 +174,7 @@ (mf/defc change-list* {::mf/private true} - [{:keys [groups conflicts selected excluded reviewed collapsed + [{:keys [groups conflicts cascades selected excluded reviewed collapsed on-select on-toggle on-fold]}] [:* (when (seq conflicts) @@ -174,7 +194,7 @@ (for [{:keys [id name rows boards count] :as group} groups] (let [open? (not (contains? collapsed id)) row-keys (into #{} (comp (filter (fn [[_ it]] (selectable-item? it))) - (map (fn [[_ it]] (:key it)))) + (mapcat (fn [[idx it]] (row-keys it (get cascades idx))))) (concat rows (mapcat :rows boards))) included? (not-any? excluded row-keys)] [:div {:class (stl/css :group) :key id} @@ -192,6 +212,7 @@ [:* (for [[idx item] rows] [:> item-row* {:key idx :index idx :item item + :inner (get cascades idx) :selected selected :excluded (contains? excluded (:key item)) :reviewed (contains? reviewed (review-key item)) @@ -203,6 +224,7 @@ :on-select on-select :on-fold on-fold} (for [[idx item] (:rows board)] [:> item-row* {:key idx :index idx :item item + :inner (get cascades idx) :selected selected :excluded (contains? excluded (:key item)) :reviewed (contains? reviewed (review-key item)) @@ -339,8 +361,8 @@ (mf/defc item-detail* {::mf/private true} - [{:keys [item items indexed excluded excluded-keys reviewed selected open-side incoming - data diff-meta on-resolve on-select on-toggle expanded on-expand]}] + [{:keys [item items indexed cascades excluded excluded-keys reviewed selected open-side + incoming data diff-meta on-resolve on-select on-toggle expanded on-expand]}] (let [type-lbl (brc/type-label-key item) cat (brc/category item) attrs (:changed-attrs item) @@ -403,7 +425,8 @@ (case (:kind item) :page (when (contains? #{:added :deleted} (:status item)) - (let [rows (filterv (fn [[_ it]] (= (:id item) (:page-id it))) indexed)] + (let [rows (filterv (fn [[_ it]] (= (:id item) (:page-id it))) indexed) + rows (remove-folded rows cascades)] [:> brf/page-facts* {:item item :data data :open-side open-side @@ -411,6 +434,7 @@ :change-count (count rows)} (for [[idx it] rows] [:> item-row* {:key idx :index idx :item it + :inner (get cascades idx) :selected selected :excluded (contains? excluded-keys (:key it)) :reviewed (contains? reviewed (review-key it)) @@ -419,6 +443,7 @@ :shape (when (contains? #{:added :deleted} (:status item)) [:> brf/shape-facts* {:item item :items items :data data + :inner (get cascades (:index selected)) :open-side open-side :incoming incoming}]) (:component :color :typography :token) @@ -428,8 +453,8 @@ (mf/defc board-detail* {::mf/private true} - [{:keys [page-id board-id groups items open-side data diff-meta selected excluded reviewed - on-select on-toggle]}] + [{:keys [page-id board-id groups items cascades open-side data diff-meta selected excluded + reviewed on-select on-toggle]}] (let [group (some #(when (= page-id (:page-id %)) %) groups) board (some #(when (= board-id (:id %)) %) (:boards group)) rows (:rows board) @@ -467,6 +492,7 @@ [:div {:class (stl/css :inner-list)} (for [[idx item] rows] [:> item-row* {:key idx :index idx :item item + :inner (get cascades idx) :selected selected :excluded (contains? excluded (:key item)) :reviewed (contains? reviewed (review-key item)) @@ -503,8 +529,16 @@ (into (vec (:changes diff)) (:conflicts diff))) indexed (mf/with-memo [items] (into [] (map-indexed vector) items)) - counts (mf/with-memo [items] - (group-by brc/category items)) + ;; the layers added or deleted with a container fold under it: + ;; the list, the filters and the counts show the container alone + cascades (mf/with-memo [indexed] + (brc/fold-cascades indexed)) + listed (mf/with-memo [indexed cascades] + (remove-folded indexed cascades)) + listed-items (mf/with-memo [listed] + (mapv second listed)) + counts (mf/with-memo [listed-items] + (group-by brc/category listed-items)) filter* (mf/use-state :all) active-filter (deref filter*) @@ -518,13 +552,13 @@ collapsed* (mf/use-state #{}) collapsed (deref collapsed*) - visible (mf/with-memo [indexed active-filter excluded] + visible (mf/with-memo [listed active-filter excluded] (filterv (fn [[_ item]] (case active-filter :all true :left-out (contains? excluded (:key item)) (= active-filter (brc/category item)))) - indexed)) + listed)) conflicts (mf/with-memo [visible] (filterv (fn [[_ it]] (= :conflict (:status it))) visible)) decisions (count (filter #(= :conflict (:status %)) items)) @@ -535,10 +569,12 @@ order (mf/with-memo [conflicts groups] (display-order conflicts groups)) - all-keys (into #{} (comp (filter selectable-item?) (map :key)) items) - kept-count (count (remove excluded all-keys)) - left-out (count (filter excluded all-keys)) - reviewed-n (count (filter #(contains? reviewed (review-key %)) items)) + ;; a folded layer travels or stays with its container, so the + ;; counts are of the rows the list shows + row-keys* (into #{} (comp (filter selectable-item?) (map :key)) listed-items) + kept-count (count (remove excluded row-keys*)) + left-out (count (filter excluded row-keys*)) + reviewed-n (count (filter #(contains? reviewed (review-key %)) listed-items)) sel-index (when (= :item (:kind selected)) (:index selected)) sel-item (when (and (some? sel-index) (< sel-index (count items))) @@ -637,7 +673,7 @@ :filters (when (and (= status :loaded) (seq items) (not expanded)) (mf/html [:* - [:> filter-chip* {:label (dm/str (tr "labels.all") " (" (count items) ")") + [:> filter-chip* {:label (dm/str (tr "labels.all") " (" (count listed-items) ")") :is-selected (= active-filter :all) :on-click #(on-filter :all)}] (for [[cat tone label icon] brc/categories @@ -658,6 +694,7 @@ (mf/html [:> change-list* {:groups groups :conflicts conflicts + :cascades cascades :selected selected :excluded excluded :reviewed reviewed @@ -685,6 +722,7 @@ :board-id (:board-id selected) :groups groups :items items + :cascades cascades :open-side open-side :data data :diff-meta diff-meta @@ -699,6 +737,7 @@ :item sel-item :items items :indexed indexed + :cascades cascades :excluded (contains? excluded (:key sel-item)) :excluded-keys excluded :reviewed reviewed @@ -729,7 +768,7 @@ (str/join " - " (remove nil? [(tr "workspace.branches.review.footer.reviewed" - (str reviewed-n) (str (count items))) + (str reviewed-n) (str (count listed-items))) (when (pos? decisions) (tr "workspace.branches.review.footer.blocked" (i18n/c decisions))) (when (pos? left-out) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs index 571afd1f50..1cf322ddfe 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs @@ -136,8 +136,9 @@ (mf/defc shape-facts* "A new or deleted layer: where it lands (or was), what it is and, for a - deleted main component, the copies it leaves behind." - [{:keys [item items data open-side incoming]}] + deleted main component, the copies it leaves behind. `inner` are the + layers added or deleted with it (`brc/fold-cascades`)." + [{:keys [item items inner data open-side incoming]}] (let [open-objs (get-in data [:pages-index (:page-id item) :objects]) ;; a layer the open file lacks is read on the other side, where ;; the changes of `items` put it and its deleted parents back @@ -161,7 +162,11 @@ "workspace.branches.review.facts.where-was" "workspace.branches.review.facts.where-lands")) (str/join " › " (cons page (brc/shape-path objects (:id item))))]] - (shape-rows shape data type-lbl))}] + (cond-> (shape-rows shape data type-lbl) + (seq inner) + (conj [(tr "workspace.branches.review.facts.contents") + (tr "workspace.branches.review.facts.layers" + (i18n/c (count inner)))])))}] (when (and copies (pos? copies)) [:> brl/notice* {:level :warning :title (tr "workspace.branches.review.facts.copies-orphaned-title")} diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index edc4c830e6..fab87a04a3 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -211,10 +211,18 @@ ;; what goes in and what stays out, when the compare dialog of this ;; branch is the one that asked for the merge diff-st (mf/deref branch-diff) + ;; counted as the compare list shows them: the layers added or + ;; deleted with a container go with it (`brc/fold-cascades`) changes (when (and (= (:id branch) (:branch-id diff-st)) (= :loaded (:status diff-st))) - (filterv #(contains? #{:added :modified :deleted} (:status %)) - (get-in diff-st [:diff :changes]))) + (let [indexed (into [] (map-indexed vector) (get-in diff-st [:diff :changes])) + folded (into #{} (comp (mapcat val) (map first)) + (brc/fold-cascades indexed))] + (into [] + (comp (remove (fn [[idx _]] (contains? folded idx))) + (map second) + (filter #(contains? #{:added :modified :deleted} (:status %)))) + indexed))) left-out (count (filter #(contains? (or excluded #{}) (:key %)) changes)) going-in (- (count changes) left-out) diff --git a/frontend/test/frontend_tests/ui/branch_review_common_test.cljs b/frontend/test/frontend_tests/ui/branch_review_common_test.cljs index 10d466ace9..710280750d 100644 --- a/frontend/test/frontend_tests/ui/branch_review_common_test.cljs +++ b/frontend/test/frontend_tests/ui/branch_review_common_test.cljs @@ -142,3 +142,29 @@ (t/is (= :components (brc/category {:kind :shape :status :modified :component? true}))) (t/is (= #{"neutral"} (set (vals (dissoc brc/category->tone :decisions))))) (t/is (= "warning" (brc/category->tone :decisions)))) + +(defn- deleted + [id parent-id] + {:kind :shape :status :deleted :id id :page-id page-id + :shape {:id id :parent-id parent-id}}) + +(t/deftest a-deleted-group-takes-its-layers-along + (let [rows [[0 (deleted :path-a :icon)] + [1 (deleted :bg :icon)] + [2 (deleted :path-b :inner)] + [3 (deleted :inner :icon)] + [4 (deleted :icon :button)] + [5 {:kind :shape :status :modified :id :label :page-id page-id}]] + folds (brc/fold-cascades rows)] + (t/is (= [4] (keys folds)) "the outermost deleted layer holds them all") + (t/is (= #{0 1 2 3} (set (map first (get folds 4))))))) + +(t/deftest only-the-same-change-folds + (let [added {:kind :shape :status :added :id :new :page-id page-id + :shape {:id :new :parent-id :icon}} + rows [[0 (deleted :icon :button)] + [1 added] + [2 (deleted :lone :button)] + [3 (assoc (deleted :other :icon) :page-id :elsewhere)]]] + (t/is (= {} (brc/fold-cascades rows)) + "an addition, a layer whose parent stays and one on another page stay apart"))) From d10380508698f0b8baba6ba22dad2d09a58d57ef Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Wed, 28 Oct 2026 09:50:32 +0100 Subject: [PATCH 3/4] :bug: Round the numbers of the compare dialog once a plugin has loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sizes, positions and other numbers in the compare dialog showed every decimal (162.99999952316284 × 42.99999808520081) as soon as a plugin had loaded. The plugin runtime's SES lockdown turns `Number.prototype.toLocaleString` into `toString`, which ignores the two-decimal limit and the locale. The numbers now go through `Intl.NumberFormat`, which the lockdown leaves alone, and fall back to the default locale when none is set. --- .../ui/workspace/sidebar/branch_review/common.cljs | 12 ++++++++---- .../ui/branches_compare_labels_test.cljs | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs index cf978891c0..e3c0b021cf 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs @@ -190,11 +190,15 @@ (defn- fmt-number "At most two decimals, trailing zeros stripped (89.99999 -> \"90\", 439.4999 -> \"439.5\", 386 -> \"386\"), grouped and punctuated the way - the active locale writes numbers (\"439,5\" in Spanish)." + the active locale writes numbers (\"439,5\" in Spanish). It goes + through `Intl.NumberFormat`: once a plugin has loaded, the SES lockdown + turns `Number.prototype.toLocaleString` into `toString`, options and + all." [v] - (let [locale (-> i18n/*current-locale* - (str/replace "_" "-"))] - (.toLocaleString v locale #js {:maximumFractionDigits 2}))) + (let [locale (or (some-> i18n/*current-locale* + (str/replace "_" "-")) + js/undefined)] + (.format (js/Intl.NumberFormat. locale #js {:maximumFractionDigits 2}) v))) (defn- short-str [v] diff --git a/frontend/test/frontend_tests/ui/branches_compare_labels_test.cljs b/frontend/test/frontend_tests/ui/branches_compare_labels_test.cljs index f5363d4c72..4fe8f2058c 100644 --- a/frontend/test/frontend_tests/ui/branches_compare_labels_test.cljs +++ b/frontend/test/frontend_tests/ui/branches_compare_labels_test.cljs @@ -69,3 +69,17 @@ (t/is (= "439.5" (display-val 439.5))) (t/is (= "90" (display-val 89.99999)) "at most two decimals") (t/is (= "386" (display-val 386))))) + +(t/deftest numbers-keep-two-decimals-under-the-plugin-lockdown + ;; a loaded plugin's SES lockdown turns `toLocaleString` into + ;; `toString`, which ignores the options + (let [proto (.-prototype js/Number) + original (.-toLocaleString proto)] + (set! (.-toLocaleString proto) (.-toString proto)) + (try + (binding [i18n/*current-locale* "en"] + (t/is (= "163" (display-val 162.99999952316284)))) + (binding [i18n/*current-locale* nil] + (t/is (= "1.5" (display-val 1.5)) "no locale falls back to the default")) + (finally + (set! (.-toLocaleString proto) original))))) From d7ede341c7158bfdb75d9bfcf4cb31d2681c38cb Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Wed, 28 Oct 2026 09:50:32 +0100 Subject: [PATCH 4/4] :sparkles: Say what goes with an added or deleted layer When a layer goes, Penpot also removes the prototype interactions that lead to it and the flows that start on it, and resizes the boards with a layout around it. The diff lists those changes as entries of their own, with nothing that ties them to their cause. The detail pane of an added or deleted layer now ends with what goes (or comes) with it: - Interactions that led into it and the flows that started on it, as warnings, naming the layers and flows they belong to. - Each board with a flex or grid layout around it, and how it resizes, or how many of its other layers move when its size is fixed. - The names of the layers folded under it. Added and deleted flow entries carry the flow as `:flow`, as shape entries carry `:shape`, so the pane can name the board a deleted flow started on. --- common/src/app/common/files/branch_merge.cljc | 19 +++- .../common_tests/files_branch_merge_test.cljc | 14 +++ .../sidebar/branch_review/common.cljs | 94 +++++++++++++++++++ .../sidebar/branch_review/facts.cljs | 79 +++++++++++++--- .../ui/branch_review_common_test.cljs | 71 ++++++++++++++ frontend/translations/en.po | 30 ++++++ 6 files changed, 291 insertions(+), 16 deletions(-) diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index fc0d34ffe1..234dda71d6 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -783,11 +783,22 @@ (sub-of ours pid :guides) {:kind :page-guide :page-id pid})) common) + ;; an added or deleted flow carries itself whole as `:flow`, as + ;; for shapes: the compare panel names the board it starts on, + ;; which the open file may lack flows-diffs (map (fn [pid] - (three-way-entities (sub-of base pid :flows) - (sub-of theirs pid :flows) - (sub-of ours pid :flows) - {:kind :page-flow :page-id pid})) + (let [bf (sub-of base pid :flows) + of (sub-of ours pid :flows)] + (update (three-way-entities bf + (sub-of theirs pid :flows) + of + {:kind :page-flow :page-id pid}) + :changes + (partial mapv (fn [e] + (case (:status e) + :added (assoc e :flow (get of (:id e))) + :deleted (assoc e :flow (get bf (:id e))) + e)))))) common) ;; default-grids per common page (mergeable: :page-grid -> :set-default-grid) grids-diffs (map (fn [pid] diff --git a/common/test/common_tests/files_branch_merge_test.cljc b/common/test/common_tests/files_branch_merge_test.cljc index cbb430f593..d3bd965f1a 100644 --- a/common/test/common_tests/files_branch_merge_test.cljc +++ b/common/test/common_tests/files_branch_merge_test.cljc @@ -170,6 +170,20 @@ (t/is (= {:id :s2 :name "B" :selrect {:x 2}} (get-in update [:s2 :shape]))) (t/is (= {:id :s3 :name "C" :selrect {:x 3}} (get-in update [:s3 :shape]))))) +(t/deftest a-deleted-flow-carries-the-board-it-starts-on + (let [flow {:id :f1 :name "Onboarding" :starting-frame :b1} + base (assoc-in (mkdata {:b1 {:id :b1 :name "Home"}}) + [:pages-index :p1 :flows] {:f1 flow}) + branch (assoc-in base [:pages-index :p1 :flows] {}) + merge (first (filter #(= :page-flow (:kind %)) + (:changes (bm/compute-merge base base branch :branch->main)))) + update (first (filter #(= :page-flow (:kind %)) + (:changes (bm/compute-merge base branch base :main->branch))))] + (t/is (= :deleted (:status merge))) + (t/is (= flow (:flow merge))) + (t/is (= :deleted (:status update))) + (t/is (= flow (:flow update))))) + (t/deftest modified-carries-no-shape (let [base (mkdata {:s1 {:id :s1 :name "A" :fill "red"}}) branch (mkdata {:s1 {:id :s1 :name "A" :fill "blue"}}) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs index e3c0b021cf..b3b5b67538 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs @@ -420,6 +420,100 @@ {} indexed))) +;; --- What goes with an added or deleted layer + +(defn- lost-interactions + "The interactions of modified entry `entry` that lead into `ids` on the + `from` side and are gone on the `to` side." + [entry ids from to] + (let [values (get-in entry [:changed-attrs :interactions]) + kept (set (get values to))] + (filterv #(and (contains? ids (:destination %)) (not (contains? kept %))) + (get values from)))) + +(defn- size-of + "Width and height of `shape` on `side`, taking the changed ones from + diff entry `entry`." + [shape entry side] + (let [attrs (:changed-attrs entry)] + [(get-in attrs [:width side] (:width shape)) + (get-in attrs [:height side] (:height shape))])) + +(defn shape-effects + "What else changes with the added or deleted layer `item`, read from + the other entries of the diff (`items`): + + `:interactions` the layers whose prototype interactions led into it + and go with it (`{:id :label :count}`); + `:flows` the flows that started on it and go with it (names); + `:layouts` the boards with a layout around it that resize + (`{:id :name :layout :from [w h] :to [w h]}`) or + whose other layers move (`{... :moved n}`). + + Penpot makes these changes itself when a layer goes, so the diff lists + them as entries of their own; this ties them to their cause. `objects` + is the page as the side that holds the layer has it, `inner` the + layers folded under it (`fold-cascades`), and `incoming` says the + changes are main's, going to the branch, rather than the branch's + going to main." + [item items objects inner incoming] + (let [from (if incoming :branch :main) + to (if incoming :main :branch) + deleted? (= :deleted (:status item)) + ids (into #{(:id item)} (map (comp :id second)) inner) + page-id (:page-id item) + on-page (filterv #(= page-id (:page-id %)) items) + modified (into {} + (comp (filter #(and (= :shape (:kind %)) (= :modified (:status %)))) + (map (juxt :id identity))) + on-page) + ancestors (loop [id (:parent-id (get objects (:id item))) out [] depth 0] + (let [shape (get objects id)] + (if (and shape (not= id uuid/zero) (< depth 64)) + (recur (:parent-id shape) (conj out shape) (inc depth)) + out)))] + {:interactions + (if deleted? + (into [] + (keep (fn [[id entry]] + (when-not (contains? ids id) + (let [lost (lost-interactions entry ids from to)] + (when (seq lost) + {:id id :label (:label entry) :count (count lost)}))))) + modified) + []) + + :flows + (if deleted? + (into [] + (keep (fn [entry] + (when (and (= :page-flow (:kind entry)) + (= :deleted (:status entry)) + (contains? ids (get-in entry [:flow :starting-frame]))) + (or (get-in entry [:flow :name]) (:label entry))))) + on-page) + []) + + :layouts + (into [] + (keep (fn [{:keys [id layout] :as shape}] + (when (some? layout) + (let [entry (get modified id) + before (size-of shape entry from) + after (size-of shape entry to) + moved (count (filter (fn [[sid e]] + (and (= id (:parent-id (get objects sid))) + (not (contains? ids sid)) + (some #(contains? (:changed-attrs e) %) [:x :y]))) + modified))] + (cond + (not= before after) + {:id id :name (:name shape) :layout layout :from before :to after} + + (pos? moved) + {:id id :name (:name shape) :layout layout :moved moved}))))) + ancestors)})) + (defn group-by-place "Group the clean changes (`[index item]` pairs) by page, and inside a page by the top-level board that holds them, reading names from the diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs index 1cf322ddfe..035f2473a8 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs @@ -32,13 +32,22 @@ "workspace.branches.review.side.main" "workspace.branches.review.side.branch"))) +(defn- names-line + "`names` joined, the first `max-names` of them and a count of the rest." + [names] + (dm/str (str/join ", " (take max-names names)) + (when (> (count names) max-names) + (dm/str " " (tr "workspace.branches.review.summary.more" + (i18n/c (- (count names) max-names))))))) + (mf/defc fact-list* "Label and value rows, one per fact." {::mf/private true} [{:keys [rows]}] [:div {:class (stl/css :facts)} - (for [[label value] (remove nil? rows)] - [:div {:class (stl/css :fact) :key label} + ;; keyed by place: a label may repeat (a layout row per board) + (for [[idx [label value]] (map-indexed vector (remove nil? rows))] + [:div {:class (stl/css :fact) :key idx} [:> text* {:as "span" :typography t/body-small :class (stl/css :fact-label)} label] [:div {:class (stl/css :fact-value)} value]])]) @@ -74,10 +83,7 @@ (tr "workspace.branches.review.facts.layers" (i18n/c layers))))] (when (seq names) [(tr "workspace.branches.review.facts.board-names") - (dm/str (str/join ", " (take max-names names)) - (when (> (count names) max-names) - (dm/str " " (tr "workspace.branches.review.summary.more" - (i18n/c (- (count names) max-names))))))])]}] + (names-line names)])]}] (when (and deleted? (pos? layers)) [:> brl/notice* {:level :warning :title (tr "workspace.branches.review.facts.page-goes")} @@ -134,6 +140,57 @@ component (conj [(tr "workspace.branches.compare.attr.component") component])))) +;; --- What goes with a layer + +(defn- size-line + [[w h]] + (dm/str (brc/display-val w) " × " (brc/display-val h))) + +(defn- layout-row + "A board with a layout around the layer: how it resizes, or how many + of its other layers move." + [{:keys [name layout from to moved]}] + [(tr "workspace.branches.review.category.layout") + (dm/str name " (" (tr (if (= :grid layout) "labels.grid-layout" "labels.flex-layout")) ") - " + (if moved + (tr "workspace.branches.review.effects.moved" (i18n/c moved)) + (dm/str (size-line from) " → " (size-line to))))]) + +(mf/defc effects* + "What else changes with an added or deleted layer (`brc/shape-effects`): + the prototype links and flows that go with it first, as warnings, then + the layouts around it that adapt and the layers it holds. Nothing + shows when none of it applies." + {::mf/private true} + [{:keys [item items objects inner incoming]}] + (let [{:keys [interactions flows layouts]} + (mf/with-memo [item items objects inner incoming] + (brc/shape-effects item items objects inner incoming)) + names (mapv (comp :label second) inner) + deleted? (= :deleted (:status item)) + rows (cond-> (mapv layout-row layouts) + (seq names) + (conj [(tr "workspace.branches.review.facts.contents") (names-line names)]))] + (when (or (seq interactions) (seq flows) (seq rows)) + [:div {:class (stl/css :section)} + [:> section-title* {} + (tr (if deleted? + "workspace.branches.review.effects.title-deleted" + "workspace.branches.review.effects.title-added"))] + (when (seq interactions) + [:> brl/notice* {:level :warning + :title (tr "workspace.branches.review.effects.interactions" + (i18n/c (reduce + (map :count interactions))))} + (tr "workspace.branches.review.effects.interactions-from" + (names-line (map :label interactions)))]) + (when (seq flows) + [:> brl/notice* {:level :warning + :title (tr "workspace.branches.review.effects.flows" + (i18n/c (count flows)))} + (names-line flows)]) + (when (seq rows) + [:> fact-list* {:rows rows}])]))) + (mf/defc shape-facts* "A new or deleted layer: where it lands (or was), what it is and, for a deleted main component, the copies it leaves behind. `inner` are the @@ -162,18 +219,16 @@ "workspace.branches.review.facts.where-was" "workspace.branches.review.facts.where-lands")) (str/join " › " (cons page (brc/shape-path objects (:id item))))]] - (cond-> (shape-rows shape data type-lbl) - (seq inner) - (conj [(tr "workspace.branches.review.facts.contents") - (tr "workspace.branches.review.facts.layers" - (i18n/c (count inner)))])))}] + (shape-rows shape data type-lbl))}] (when (and copies (pos? copies)) [:> brl/notice* {:level :warning :title (tr "workspace.branches.review.facts.copies-orphaned-title")} (tr (if incoming "workspace.branches.review.facts.copies-orphaned-update" "workspace.branches.review.facts.copies-orphaned-merge") - (i18n/c copies))])]))) + (i18n/c copies))]) + [:> effects* {:item item :items items :objects objects + :inner inner :incoming incoming}]]))) ;; --- Library entries other layers depend on diff --git a/frontend/test/frontend_tests/ui/branch_review_common_test.cljs b/frontend/test/frontend_tests/ui/branch_review_common_test.cljs index 710280750d..fbfeb60a9c 100644 --- a/frontend/test/frontend_tests/ui/branch_review_common_test.cljs +++ b/frontend/test/frontend_tests/ui/branch_review_common_test.cljs @@ -168,3 +168,74 @@ [3 (assoc (deleted :other :icon) :page-id :elsewhere)]]] (t/is (= {} (brc/fold-cascades rows)) "an addition, a layer whose parent stays and one on another page stay apart"))) + +;; --- What goes with an added or deleted layer + +(def ^:private home-id (uuid/next)) +(def ^:private screen-id (uuid/next)) +(def ^:private cta-id (uuid/next)) +(def ^:private button-id (uuid/next)) +(def ^:private icon-id (uuid/next)) +(def ^:private label-id (uuid/next)) + +(def ^:private proto-objects + "Main's page: a button with a flex layout on Home, and a Screen the + CTA leads to." + {uuid/zero {:id uuid/zero :type :frame :shapes [home-id screen-id]} + home-id {:id home-id :type :frame :name "Home" :parent-id uuid/zero} + screen-id {:id screen-id :type :frame :name "Screen" :parent-id uuid/zero} + cta-id {:id cta-id :type :rect :name "CTA" :parent-id home-id} + button-id {:id button-id :type :frame :name "Button" :parent-id home-id + :layout :flex :width 163 :height 48} + icon-id {:id icon-id :type :group :name "icon" :parent-id button-id} + label-id {:id label-id :type :text :name "label" :parent-id button-id}}) + +(defn- gone + [id] + {:kind :shape :status :deleted :id id :page-id page-id}) + +(defn- edit + [id attrs] + {:kind :shape :status :modified :id id :page-id page-id :label (name id) + :changed-attrs attrs}) + +(t/deftest a-board-with-a-layout-resizes-with-the-layer + (let [items [(gone icon-id) + (edit button-id {:width {:main 163 :branch 131}})] + fx (brc/shape-effects (gone icon-id) items proto-objects [] false)] + (t/is (= [{:id button-id :name "Button" :layout :flex :from [163 48] :to [131 48]}] + (:layouts fx))) + (t/is (= [] (:interactions fx))) + (t/is (= [] (:flows fx))))) + +(t/deftest an-update-reads-the-sides-the-other-way-round + (let [items [(gone icon-id) + (edit button-id {:width {:main 131 :branch 163}})] + fx (brc/shape-effects (gone icon-id) items proto-objects [] true)] + (t/is (= [[163 48] [131 48]] ((juxt :from :to) (first (:layouts fx))))))) + +(t/deftest a-fixed-board-moves-its-other-layers + (let [items [(gone icon-id) + (edit label-id {:x {:main 40 :branch 20}})] + fx (brc/shape-effects (gone icon-id) items proto-objects [] false)] + (t/is (= [{:id button-id :name "Button" :layout :flex :moved 1}] (:layouts fx))))) + +(t/deftest prototype-links-into-a-deleted-board-go-with-it + (let [link {:event-type :click :action-type :navigate :destination screen-id} + items [(gone screen-id) + (assoc (edit cta-id {:interactions {:main [link] :branch []}}) :label "CTA") + {:kind :page-flow :status :deleted :id :f1 :page-id page-id :label "f1" + :flow {:id :f1 :name "Onboarding" :starting-frame screen-id}} + {:kind :page-flow :status :deleted :id :f2 :page-id page-id :label "f2" + :flow {:id :f2 :name "Other" :starting-frame home-id}}] + fx (brc/shape-effects (gone screen-id) items proto-objects [] false)] + (t/is (= [{:id cta-id :label "CTA" :count 1}] (:interactions fx))) + (t/is (= ["Onboarding"] (:flows fx))) + (t/is (= [] (:layouts fx)) "the page root is not a layout"))) + +(t/deftest an-added-layer-breaks-no-links + (let [link {:event-type :click :action-type :navigate :destination screen-id} + added (assoc (gone screen-id) :status :added) + items [added (edit cta-id {:interactions {:main [] :branch [link]}})] + fx (brc/shape-effects added items proto-objects [] false)] + (t/is (= [] (:interactions fx))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 6aae6ad1c5..f8281f58c9 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -11536,6 +11536,36 @@ msgid_plural "workspace.branches.review.facts.deleted-used" msgstr[0] "1 layer in this file loses its link to it." msgstr[1] "%s layers in this file lose their link to it." +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.title-deleted" +msgstr "What goes with it" + +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.title-added" +msgstr "What comes with it" + +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.interactions" +msgid_plural "workspace.branches.review.effects.interactions" +msgstr[0] "1 prototype interaction leads here and goes too" +msgstr[1] "%s prototype interactions lead here and go too" + +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.interactions-from" +msgstr "On %s" + +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.flows" +msgid_plural "workspace.branches.review.effects.flows" +msgstr[0] "1 prototype flow starts here and goes too" +msgstr[1] "%s prototype flows start here and go too" + +#: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +msgid "workspace.branches.review.effects.moved" +msgid_plural "workspace.branches.review.effects.moved" +msgstr[0] "1 other layer moves" +msgstr[1] "%s other layers move" + #: src/app/main/ui/workspace/sidebar/branch_review/common.cljs msgid "workspace.branches.review.group.pages" msgstr "Pages"