feat: prefer user defined coords and dimensions over calculated for for frame (#8517)

* feat: prefer user defined coords and dimensions over calculated for frame

* update changelog

* lint

* show the info only in dev mode and when children present

packages/excalidraw/CHANGELOG.md

@@ -15,6 +15,8 @@ Please add the latest change on the top under the correct section.
 
 ### Features
 
+- Prefer user defined coordinates and dimensions when creating a frame using [`convertToExcalidrawElements`](https://docs.excalidraw.com/docs/@excalidraw/excalidraw/api/excalidraw-element-skeleton#converttoexcalidrawelements) [#8517](https://github.com/excalidraw/excalidraw/pull/8517)
+
 - `props.initialData` can now be a function that returns `ExcalidrawInitialDataState` or `Promise<ExcalidrawInitialDataState>`. [#8107](https://github.com/excalidraw/excalidraw/pull/8135)
 
 - Added support for multiplayer undo/redo, by calculating invertible increments and storing them inside the local-only undo/redo stacks. [#7348](https://github.com/excalidraw/excalidraw/pull/7348)

packages/excalidraw/data/__snapshots__/transform.test.ts.snap

@@ -6,11 +6,11 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s
   "backgroundColor": "#d8f5a2",
   "boundElements": [
     {
-      "id": "id45",
+      "id": "id47",
       "type": "arrow",
     },
     {
-      "id": "id46",
+      "id": "id48",
       "type": "arrow",
     },
   ],
@@ -47,7 +47,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id46",
+      "id": "id48",
       "type": "arrow",
     },
   ],
@@ -118,7 +118,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s
   "seed": Any<Number>,
   "startArrowhead": null,
   "startBinding": {
-    "elementId": "id47",
+    "elementId": "id49",
     "fixedPoint": null,
     "focus": -0.08139534883720931,
     "gap": 1,
@@ -200,7 +200,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id45",
+      "id": "id47",
       "type": "arrow",
     },
   ],
@@ -238,7 +238,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing t
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id48",
+      "id": "id50",
       "type": "arrow",
     },
   ],
@@ -284,7 +284,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing t
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id48",
+      "id": "id50",
       "type": "arrow",
     },
   ],
@@ -329,7 +329,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing t
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id49",
+      "id": "id51",
       "type": "text",
     },
   ],
@@ -392,7 +392,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing t
   "autoResize": true,
   "backgroundColor": "transparent",
   "boundElements": null,
-  "containerId": "id48",
+  "containerId": "id50",
   "customData": undefined,
   "fillStyle": "solid",
   "fontFamily": 5,
@@ -433,7 +433,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id38",
+      "id": "id40",
       "type": "text",
     },
   ],
@@ -441,7 +441,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "elbowed": false,
   "endArrowhead": "arrow",
   "endBinding": {
-    "elementId": "id40",
+    "elementId": "id42",
     "fixedPoint": null,
     "focus": 0,
     "gap": 1,
@@ -472,7 +472,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "seed": Any<Number>,
   "startArrowhead": null,
   "startBinding": {
-    "elementId": "id39",
+    "elementId": "id41",
     "fixedPoint": null,
     "focus": 0,
     "gap": 1,
@@ -496,7 +496,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "autoResize": true,
   "backgroundColor": "transparent",
   "boundElements": null,
-  "containerId": "id37",
+  "containerId": "id39",
   "customData": undefined,
   "fillStyle": "solid",
   "fontFamily": 5,
@@ -537,7 +537,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id37",
+      "id": "id39",
       "type": "arrow",
     },
   ],
@@ -574,7 +574,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to shapes whe
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id37",
+      "id": "id39",
       "type": "arrow",
     },
   ],
@@ -611,7 +611,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id42",
+      "id": "id44",
       "type": "text",
     },
   ],
@@ -619,7 +619,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "elbowed": false,
   "endArrowhead": "arrow",
   "endBinding": {
-    "elementId": "id44",
+    "elementId": "id46",
     "fixedPoint": null,
     "focus": 0,
     "gap": 1,
@@ -650,7 +650,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "seed": Any<Number>,
   "startArrowhead": null,
   "startBinding": {
-    "elementId": "id43",
+    "elementId": "id45",
     "fixedPoint": null,
     "focus": 0,
     "gap": 1,
@@ -674,7 +674,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "autoResize": true,
   "backgroundColor": "transparent",
   "boundElements": null,
-  "containerId": "id41",
+  "containerId": "id43",
   "customData": undefined,
   "fillStyle": "solid",
   "fontFamily": 5,
@@ -716,7 +716,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id41",
+      "id": "id43",
       "type": "arrow",
     },
   ],
@@ -762,7 +762,7 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to text when
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id41",
+      "id": "id43",
       "type": "arrow",
     },
   ],
@@ -1303,7 +1303,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id54",
+      "id": "id56",
       "type": "text",
     },
     {
@@ -1346,7 +1346,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id55",
+      "id": "id57",
       "type": "text",
     },
   ],
@@ -1385,7 +1385,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id56",
+      "id": "id58",
       "type": "text",
     },
     {
@@ -1428,7 +1428,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id57",
+      "id": "id59",
       "type": "text",
     },
     {
@@ -1475,7 +1475,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id58",
+      "id": "id60",
       "type": "text",
     },
   ],
@@ -1540,7 +1540,7 @@ exports[`Test Transform > should transform the elements correctly when linear el
   "backgroundColor": "transparent",
   "boundElements": [
     {
-      "id": "id59",
+      "id": "id61",
       "type": "text",
     },
   ],

packages/excalidraw/data/transform.test.ts

@@ -309,8 +309,7 @@ describe("Test Transform", () => {
   });
 
   describe("Test Frames", () => {
-    it("should transform frames and update frame ids when regenerated", () => {
+    const elements: ExcalidrawElementSkeleton[] = [
-      const elementsSkeleton: ExcalidrawElementSkeleton[] = [
       {
         type: "rectangle",
         x: 10,
@@ -331,6 +330,11 @@ describe("Test Transform", () => {
         },
         id: "2",
       },
+    ];
+
+    it("should transform frames and update frame ids when regenerated", () => {
+      const elementsSkeleton: ExcalidrawElementSkeleton[] = [
+        ...elements,
         {
           type: "frame",
           children: ["1", "2"],
@@ -352,28 +356,9 @@ describe("Test Transform", () => {
       });
     });
 
-    it("should consider max of calculated and frame dimensions when provided", () => {
+    it("should consider user defined frame dimensions over calculated when provided", () => {
       const elementsSkeleton: ExcalidrawElementSkeleton[] = [
-        {
+        ...elements,
-          type: "rectangle",
-          x: 10,
-          y: 10,
-          strokeWidth: 2,
-          id: "1",
-        },
-        {
-          type: "diamond",
-          x: 120,
-          y: 20,
-          backgroundColor: "#fff3bf",
-          strokeWidth: 2,
-          label: {
-            text: "HELLO EXCALIDRAW",
-            strokeColor: "#099268",
-            fontSize: 30,
-          },
-          id: "2",
-        },
         {
           type: "frame",
           children: ["1", "2"],
@@ -388,7 +373,27 @@ describe("Test Transform", () => {
       );
       const frame = excalidrawElements.find((ele) => ele.type === "frame")!;
       expect(frame.width).toBe(800);
-      expect(frame.height).toBe(126);
+      expect(frame.height).toBe(100);
+    });
+
+    it("should consider user defined frame coordinates calculated when provided", () => {
+      const elementsSkeleton: ExcalidrawElementSkeleton[] = [
+        ...elements,
+        {
+          type: "frame",
+          children: ["1", "2"],
+          name: "My frame",
+          x: 100,
+          y: 300,
+        },
+      ];
+      const excalidrawElements = convertToExcalidrawElements(
+        elementsSkeleton,
+        opts,
+      );
+      const frame = excalidrawElements.find((ele) => ele.type === "frame")!;
+      expect(frame.x).toBe(100);
+      expect(frame.y).toBe(300);
     });
   });
 

packages/excalidraw/data/transform.ts

@@ -46,6 +46,7 @@ import {
   assertNever,
   cloneJSON,
   getFontString,
+  isDevEnv,
   toBrandedType,
 } from "../utils";
 import { getSizeFromPoints } from "../points";
@@ -717,7 +718,7 @@ export const convertToExcalidrawElements = (
   }
 
   // Once all the excalidraw elements are created, we can add frames since we
-  // need to calculate coordinates and dimensions of frame which is possibe after all
+  // need to calculate coordinates and dimensions of frame which is possible after all
   // frame children are processed.
   for (const [id, element] of elementsWithIds) {
     if (element.type !== "frame" && element.type !== "magicframe") {
@@ -764,10 +765,26 @@ export const convertToExcalidrawElements = (
     maxX = maxX + PADDING;
     maxY = maxY + PADDING;
 
-    // Take the max of calculated and provided frame dimensions, whichever is higher
+    const frameX = frame?.x || minX;
-    const width = Math.max(frame?.width, maxX - minX);
+    const frameY = frame?.y || minY;
-    const height = Math.max(frame?.height, maxY - minY);
+    const frameWidth = frame?.width || maxX - minX;
-    Object.assign(frame, { x: minX, y: minY, width, height });
+    const frameHeight = frame?.height || maxY - minY;
+
+    Object.assign(frame, {
+      x: frameX,
+      y: frameY,
+      width: frameWidth,
+      height: frameHeight,
+    });
+    if (
+      isDevEnv() &&
+      element.children.length &&
+      (frame?.x || frame?.y || frame?.width || frame?.height)
+    ) {
+      console.info(
+        "User provided frame attributes are being considered, if you find this inaccurate, please remove any of the attributes - x, y, width and height so frame coordinates and dimensions are calculated automatically",
+      );
+    }
   }
 
   return elementStore.getElements();