Fix bad dev-conditionally around data attributes; better handle negat…

…ive group sizes
bvaughn · Nov 13, 2023 · b55808e · b55808e
1 parent cb48f3d
commit b55808e
Show file tree

Hide file tree

Showing 9 changed files with 177 additions and 19 deletions.
diff --git a/packages/react-resizable-panels/src/Panel.ts b/packages/react-resizable-panels/src/Panel.ts
@@ -243,12 +243,12 @@ export function PanelWithForwardedRef({
 
     // CSS selectors
     "data-panel": "",
+    "data-panel-id": panelId,
 
     // e2e test attributes
     "data-panel-collapsible": isDevelopment
       ? collapsible || undefined
       : undefined,
-    "data-panel-id": isDevelopment ? panelId : undefined,
     "data-panel-size": isDevelopment
       ? parseFloat("" + style.flexGrow).toFixed(1)
       : undefined,

diff --git a/packages/react-resizable-panels/src/PanelGroup.ts b/packages/react-resizable-panels/src/PanelGroup.ts
@@ -266,6 +266,10 @@ function PanelGroupWithForwardedRef({
     }
 
     const groupSizePixels = calculateAvailablePanelSizeInPixels(groupId);
+    if (groupSizePixels <= 0) {
+      // Wait until the group has rendered a non-zero size before computing layout.
+      return;
+    }
 
     if (unsafeLayout == null) {
       unsafeLayout = calculateUnsafeDefaultLayout({
@@ -925,10 +929,8 @@ function PanelGroupWithForwardedRef({
 
       // CSS selectors
       "data-panel-group": "",
-
-      // e2e test attributes
-      "data-panel-group-direction": isDevelopment ? direction : undefined,
-      "data-panel-group-id": isDevelopment ? groupId : undefined,
+      "data-panel-group-direction": direction,
+      "data-panel-group-id": groupId,
     })
   );
 }

diff --git a/packages/react-resizable-panels/src/PanelResizeHandle.ts b/packages/react-resizable-panels/src/PanelResizeHandle.ts
@@ -151,19 +151,6 @@ export function PanelResizeHandle({
   return createElement(Type, {
     children,
     className: classNameFromProps,
-
-    // CSS selectors
-    "data-resize-handle": "",
-
-    "data-resize-handle-active": isDragging
-      ? "pointer"
-      : isFocused
-      ? "keyboard"
-      : undefined,
-    "data-panel-group-direction": direction,
-    "data-panel-group-id": groupId,
-    "data-panel-resize-handle-enabled": !disabled,
-    "data-panel-resize-handle-id": resizeHandleId,
     onBlur: () => setIsFocused(false),
     onFocus: () => setIsFocused(true),
     onMouseDown: (event: ReactMouseEvent) => {
@@ -192,6 +179,18 @@ export function PanelResizeHandle({
       ...styleFromProps,
     },
     tabIndex: 0,
+
+    // CSS selectors
+    "data-panel-group-direction": direction,
+    "data-panel-group-id": groupId,
+    "data-resize-handle": "",
+    "data-resize-handle-active": isDragging
+      ? "pointer"
+      : isFocused
+      ? "keyboard"
+      : undefined,
+    "data-panel-resize-handle-enabled": !disabled,
+    "data-panel-resize-handle-id": resizeHandleId,
   });
 }
 

diff --git a/packages/react-resizable-panels/src/utils/computePercentagePanelConstraints.test.ts b/packages/react-resizable-panels/src/utils/computePercentagePanelConstraints.test.ts
@@ -68,4 +68,31 @@ describe("computePercentagePanelConstraints", () => {
 }
 `);
   });
+
+  it("should compute reasonable percentage based constraints from pixels if group size is negative", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
+    expect(
+      computePercentagePanelConstraints(
+        [
+          {
+            minSizePixels: 25,
+            maxSizePixels: 100,
+          },
+        ],
+
+        0,
+        -100
+      )
+    ).toMatchInlineSnapshot(`
+{
+  "collapsedSizePercentage": 0,
+  "defaultSizePercentage": undefined,
+  "maxSizePercentage": 0,
+  "minSizePercentage": 0,
+}
+`);
+
+    expect(console.warn).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/packages/react-resizable-panels/src/utils/convertPixelConstraintsToPercentages.test.ts b/packages/react-resizable-panels/src/utils/convertPixelConstraintsToPercentages.test.ts
@@ -0,0 +1,47 @@
+import { convertPixelConstraintsToPercentages } from "./convertPixelConstraintsToPercentages";
+
+describe("convertPixelConstraintsToPercentages", () => {
+  it("should respect percentage panel constraints if group size is negative", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
+    expect(
+      convertPixelConstraintsToPercentages(
+        {
+          minSizePercentage: 25,
+          defaultSizePercentage: 50,
+          maxSizePercentage: 75,
+        },
+        -100
+      )
+    ).toEqual({
+      collapsedSizePercentage: 0,
+      defaultSizePercentage: 50,
+      maxSizePercentage: 75,
+      minSizePercentage: 25,
+    });
+
+    expect(console.warn).toHaveBeenCalledTimes(0);
+  });
+
+  // Edge case test (issues/206)
+  it("should ignore pixel panel constraints if group size is negative", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
+    expect(
+      convertPixelConstraintsToPercentages(
+        {
+          minSizePixels: 25,
+          maxSizePixels: 75,
+        },
+        -100
+      )
+    ).toEqual({
+      collapsedSizePercentage: 0,
+      defaultSizePercentage: undefined,
+      maxSizePercentage: 0,
+      minSizePercentage: 0,
+    });
+
+    expect(console.warn).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/packages/react-resizable-panels/src/utils/convertPixelConstraintsToPercentages.ts b/packages/react-resizable-panels/src/utils/convertPixelConstraintsToPercentages.ts
@@ -21,6 +21,23 @@ export function convertPixelConstraintsToPercentages(
     minSizePixels,
   } = panelConstraints;
 
+  const hasPixelConstraints =
+    collapsedSizePixels != null ||
+    defaultSizePixels != null ||
+    minSizePixels != null ||
+    maxSizePixels != null;
+
+  if (hasPixelConstraints && groupSizePixels <= 0) {
+    console.warn(`WARNING: Invalid group size: ${groupSizePixels}px`);
+
+    return {
+      collapsedSizePercentage: 0,
+      defaultSizePercentage,
+      maxSizePercentage: 0,
+      minSizePercentage: 0,
+    };
+  }
+
   if (collapsedSizePixels != null) {
     collapsedSizePercentage = convertPixelsToPercentage(
       collapsedSizePixels,

diff --git a/packages/react-resizable-panels/src/utils/dom/getPanelGroupElement.ts b/packages/react-resizable-panels/src/utils/dom/getPanelGroupElement.ts
@@ -1,5 +1,7 @@
 export function getPanelGroupElement(id: string): HTMLDivElement | null {
-  const element = document.querySelector(`[data-panel-group-id="${id}"]`);
+  const element = document.querySelector(
+    `[data-panel-group][data-panel-group-id="${id}"]`
+  );
   if (element) {
     return element as HTMLDivElement;
   }

diff --git a/packages/react-resizable-panels/src/utils/resizePanel.test.ts b/packages/react-resizable-panels/src/utils/resizePanel.test.ts
@@ -0,0 +1,45 @@
+import { resizePanel } from "./resizePanel";
+
+describe("resizePanel", () => {
+  // Edge case test (issues/206)
+  fit("should respect percentage panel constraints if group size is negative", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
+    expect(
+      resizePanel({
+        groupSizePixels: -100,
+        panelConstraints: [
+          {
+            minSizePercentage: 25,
+            maxSizePercentage: 75,
+          },
+        ],
+        panelIndex: 0,
+        size: 50,
+      })
+    ).toBe(50);
+
+    expect(console.warn).toHaveBeenCalledTimes(0);
+  });
+
+  // Edge case test (issues/206)
+  it("should handle pixel panel constraints if group size is negative", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
+    expect(
+      resizePanel({
+        groupSizePixels: -100,
+        panelConstraints: [
+          {
+            minSizePixels: 25,
+            maxSizePixels: 75,
+          },
+        ],
+        panelIndex: 0,
+        size: 50,
+      })
+    ).toBe(0);
+
+    expect(console.warn).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/packages/react-resizable-panels/src/utils/resizePanel.ts b/packages/react-resizable-panels/src/utils/resizePanel.ts
@@ -14,6 +14,25 @@ export function resizePanel({
   panelIndex: number;
   size: number;
 }) {
+  const hasPixelConstraints = panelConstraints.some(
+    ({
+      collapsedSizePixels,
+      defaultSizePixels,
+      minSizePixels,
+      maxSizePixels,
+    }) =>
+      collapsedSizePixels != null ||
+      defaultSizePixels != null ||
+      minSizePixels != null ||
+      maxSizePixels != null
+  );
+
+  if (hasPixelConstraints && groupSizePixels <= 0) {
+    console.warn(`WARNING: Invalid group size: ${groupSizePixels}px`);
+
+    return 0;
+  }
+
   let { collapsible } = panelConstraints[panelIndex]!;
 
   const { collapsedSizePercentage, maxSizePercentage, minSizePercentage } =