From b55808efcb5aa946325a07bd20739dc92d68654b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 13 Nov 2023 14:09:07 -0500 Subject: [PATCH] Fix bad dev-conditionally around data attributes; better handle negative group sizes --- packages/react-resizable-panels/src/Panel.ts | 2 +- .../react-resizable-panels/src/PanelGroup.ts | 10 ++-- .../src/PanelResizeHandle.ts | 25 +++++----- .../computePercentagePanelConstraints.test.ts | 27 +++++++++++ ...nvertPixelConstraintsToPercentages.test.ts | 47 +++++++++++++++++++ .../convertPixelConstraintsToPercentages.ts | 17 +++++++ .../src/utils/dom/getPanelGroupElement.ts | 4 +- .../src/utils/resizePanel.test.ts | 45 ++++++++++++++++++ .../src/utils/resizePanel.ts | 19 ++++++++ 9 files changed, 177 insertions(+), 19 deletions(-) create mode 100644 packages/react-resizable-panels/src/utils/convertPixelConstraintsToPercentages.test.ts create mode 100644 packages/react-resizable-panels/src/utils/resizePanel.test.ts diff --git a/packages/react-resizable-panels/src/Panel.ts b/packages/react-resizable-panels/src/Panel.ts index 451e00ae3..e94d9d84f 100644 --- 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 index 5c6bd5ca6..7b2fec053 100644 --- 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 index 5fe9b5ee0..7173967f7 100644 --- 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 index 8be84c2c4..0c8d485e6 100644 --- 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 new file mode 100644 index 000000000..1065e577c --- /dev/null +++ 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 index d4348f3ff..ce682370b 100644 --- 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 index 835fe8466..876e4e8b3 100644 --- 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 new file mode 100644 index 000000000..907a0c21e --- /dev/null +++ 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 index c8bc51789..2c27f0367 100644 --- 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 } =