Repository: penpot/penpot
File: frontend/src/app/main/data/workspace/variants.cljs (ClojureScript)

Code before:
(defn- bounding-rect
  "Receives a list of frames (with X, y, width and height) and
  calculates a rect that contains them all"
  [frames]
  (let [xs   (map :x frames)
        ys   (map :y frames)
        x2s  (map #(+ (:x %) (:width %)) frames)
        y2s  (map #(+ (:y %) (:height %)) frames)
        min-x (apply min xs)
        min-y (apply min ys)
        max-x (apply max x2s)
        max-y (apply max y2s)]
    {:x min-x
     :y min-y
     :width (- max-x min-x)
     :height (- max-y min-y)}))

(defn- common-prefix
  [paths]
  (->> (apply map vector paths)
       (take-while #(apply = %))
       (map first)
       vec))

(defn combine-as-variants
  ([options]
   (combine-as-variants nil options))
  ([selected {:keys [page-id trigger]}]
   (ptk/reify ::combine-as-variants
     ptk/WatchEvent
     (watch [_ state stream]
       (let [current-page  (:current-page-id state)

             combine
             (fn [current-page]
               (let [objects       (dsh/lookup-page-objects state current-page)
                     selected      (->> (or selected (dsh/lookup-selected state))
                                        (cfh/clean-loops objects)
                                        (remove (fn [id]
                                                  (let [shape (get objects id)]
                                                    (or (not (ctc/main-instance? shape))
                                                        (ctc/is-variant? shape))))))]
                 (when (> (count selected) 1)
                   (let [shapes        (mapv #(get objects %) selected)
                         rect          (bounding-rect shapes)
                         prefix        (->> shapes
                                            (mapv #(cfh/split-path (:name %)))
                                            (common-prefix))
                         ;; When the common parent is root, add a wrapper
                         add-wrapper?  (empty? prefix)
                         first-shape   (first shapes)

Review comment (suggestion):
Making the first argument as optional looks confusing and is unexpected on code. 

The best approach for handle this, is have two events: one high-level that takes no selected argument and deduces it from the "context"/"state", and then the lower-level event which receives the collected context (the "selected" and others) and executes the main logic.


Code after:
(defn- bounding-rect
  "Receives a list of frames (with X, y, width and height) and
  calculates a rect that contains them all"
  [frames]
  (let [xs   (map :x frames)
        ys   (map :y frames)
        x2s  (map #(+ (:x %) (:width %)) frames)
        y2s  (map #(+ (:y %) (:height %)) frames)
        min-x (apply min xs)
        min-y (apply min ys)
        max-x (apply max x2s)
        max-y (apply max y2s)]
    {:x min-x
     :y min-y
     :width (- max-x min-x)
     :height (- max-y min-y)}))

(defn- common-prefix
  [paths]
  (->> (apply map vector paths)
       (take-while #(apply = %))
       (map first)
       vec))

(defn combine-as-variants
  [ids {:keys [page-id trigger]}]
  (ptk/reify ::combine-as-variants
    ptk/WatchEvent
    (watch [_ state stream]
      (let [current-page  (:current-page-id state)

            combine
            (fn [current-page]
              (let [objects       (dsh/lookup-page-objects state current-page)
                    ids           (->> ids
                                       (cfh/clean-loops objects)
                                       (remove (fn [id]
                                                 (let [shape (get objects id)]
                                                   (or (not (ctc/main-instance? shape))
                                                       (ctc/is-variant? shape))))))]
                (when (> (count ids) 1)
                  (let [shapes        (mapv #(get objects %) ids)
                        rect          (bounding-rect shapes)
                        prefix        (->> shapes
                                           (mapv #(cfh/split-path (:name %)))
                                           (common-prefix))
                         ;; When the common parent is root, add a wrapper
                        add-wrapper?  (empty? prefix)
                        first-shape   (first shapes)
                        delta         (gpt/point (- (:x rect) (:x first-shape) 30)
                                                 (- (:y rect) (:y first-shape) 30))