From b4ec1d9d5b85e4423e44d3408a7d50b683564ec9 Mon Sep 17 00:00:00 2001 From: Preet Sidhu Date: Fri, 28 Apr 2023 15:31:29 -0400 Subject: [PATCH] fix: available columns calculation in assigning width to fill widgets (#22688) ## Description 1. On smaller viewports, space can not be equally divided between fill widgets because it is less than the min width of some. Width allocation is done in descending order of min width requirement of fill widgets. Allocation width is now being deducted from available columns to fix the logic. 2. Disable widget grouping on auto layout canvas. Fixes # 1. https://github.com/appsmithorg/appsmith/issues/22555 3. https://github.com/appsmithorg/appsmith/issues/22234 ## Type of change - Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - Manual - Jest - ## Checklist: ### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag --- app/client/src/sagas/WidgetOperationSagas.tsx | 4 +- .../utils/autoLayout/positionUtils.test.ts | 466 ++++++++++++++++++ .../src/utils/autoLayout/positionUtils.ts | 14 +- 3 files changed, 475 insertions(+), 9 deletions(-) diff --git a/app/client/src/sagas/WidgetOperationSagas.tsx b/app/client/src/sagas/WidgetOperationSagas.tsx index f9ac8048d9..717d23303d 100644 --- a/app/client/src/sagas/WidgetOperationSagas.tsx +++ b/app/client/src/sagas/WidgetOperationSagas.tsx @@ -2002,7 +2002,9 @@ function* addSuggestedWidget(action: ReduxAction>) { export function* groupWidgetsSaga() { const selectedWidgetIDs: string[] = yield select(getSelectedWidgets); const isMultipleWidgetsSelected = selectedWidgetIDs.length > 1; - + // Grouping functionality has been temporarily disabled for auto layout canvas. + const isAutoLayout: boolean = yield select(getIsAutoLayout); + if (isAutoLayout) return; if (isMultipleWidgetsSelected) { try { yield put({ diff --git a/app/client/src/utils/autoLayout/positionUtils.test.ts b/app/client/src/utils/autoLayout/positionUtils.test.ts index 9e2b5c062e..cad6e6e6a9 100644 --- a/app/client/src/utils/autoLayout/positionUtils.test.ts +++ b/app/client/src/utils/autoLayout/positionUtils.test.ts @@ -17,6 +17,8 @@ import { updateWidgetPositions, } from "./positionUtils"; import { AppPositioningTypes } from "reducers/entityReducers/pageListReducer"; +import { LabelPosition } from "components/constants"; +import * as utils from "./flexWidgetUtils"; describe("test PositionUtils methods", () => { const mainCanvasWidth = 960; @@ -166,6 +168,159 @@ describe("test PositionUtils methods", () => { .fillWidgetLength, ).toEqual(24); }); + it("should allocate columns for fill widgets that match min widths if enough space is unavailable", () => { + const widgets = { + "1": { + widgetId: "1", + leftColumn: 0, + rightColumn: 32, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 4, + type: "DOCUMENT_VIEWER_WIDGET", + widgetName: "", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Hug, + autoLayout: { + widgetSize: [ + { + viewportMinWidth: 0, + configuration: () => { + return { + minWidth: "280px", + minHeight: "280px", + }; + }, + }, + ], + }, + }, + "2": { + widgetId: "2", + leftColumn: 32, + rightColumn: 64, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 7, + type: "CURRENCY_INPUT_WIDGET", + widgetName: "Currency1", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + autoLayout: { + disabledPropsDefaults: { + labelPosition: LabelPosition.Top, + labelTextSize: "0.875rem", + }, + defaults: { + rows: 6.6, + }, + autoDimension: { + height: true, + }, + widgetSize: [ + { + viewportMinWidth: 0, + configuration: () => { + return { + minWidth: "120px", + }; + }, + }, + ], + disableResizeHandles: { + vertical: true, + }, + }, + }, + "3": { + widgetId: "3", + leftColumn: 64, + rightColumn: 96, + alignment: FlexLayerAlignment.End, + topRow: 0, + bottomRow: 7, + type: "CONTAINER_WIDGET", + widgetName: "Container1", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + }, + }; + jest + .spyOn(utils, "getWidgetMinMaxDimensionsInPixel") + // eslint-disable-next-line @typescript-eslint/no-unused-vars + .mockImplementation((widget: any, width: number) => { + if ( + ["DOCUMENT_VIEWER_WIDGET", "CONTAINER_WIDGET"].includes( + widget?.type, + ) + ) + return { + minWidth: 280, + minHeight: 280, + maxWidth: undefined, + maxHeight: undefined, + }; + if (widget?.type === "CURRENCY_INPUT_WIDGET") + return { + minWidth: 120, + minHeight: 40, + maxWidth: undefined, + maxHeight: undefined, + }; + return { + minWidth: undefined, + minHeight: undefined, + maxWidth: undefined, + maxHeight: undefined, + }; + }); + const layer: FlexLayer = { + children: [ + { id: "1", align: FlexLayerAlignment.Start }, + { id: "2", align: FlexLayerAlignment.Start }, + { id: "3", align: FlexLayerAlignment.End }, + ], + }; + const result = extractAlignmentInfo(widgets, layer, false, 600, 1, false); + + /** + * Canvas width = 600 + * # of fill widgets = 3 + * standard fill widget length (f) = 600 / 3 = 200 + * min widths in descending order -> 280, 280, 120. + * + * In descending order of min widths: + * available space: 600 + * 1st fill widget length (DocumentViewer) -> 280 > 200 -> 280 + * available space: 600 - 280 = 320 + * standard fill widget length (f) = 320 / 2 = 160 + * + * 2nd fill widget length (ContainerWidget) -> 280 > 160 -> 280 + * available space: 320 - 280 = 40 + * standard fill widget length (f) = 40 / 1 = 40 + * + * 3rd fill widget length (CurrencyInput) -> 120 > 40 -> 120 + * + * => widgets will overflow the canvas. + */ + + // DocumnetViewer + CurrencyInput + expect(result.info[0].columns).toEqual(280 + 120); + // ContainerWidget + expect(result.info[2].columns).toEqual(280); + }); }); describe("test getAlignmentSizeinfo method", () => { @@ -589,6 +744,317 @@ describe("test PositionUtils methods", () => { expect(result.widgets["3"].mobileLeftColumn).toEqual(48); expect(result.widgets["3"].mobileRightColumn).toEqual(64); }); + + it("should allocate columns for fill widgets in descending order of their min width requirement", () => { + const widgets = { + "1": { + widgetId: "1", + leftColumn: 0, + rightColumn: 32, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 4, + type: "DOCUMENT_VIEWER_WIDGET", + widgetName: "", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + autoLayout: { + widgetSize: [ + { + viewportMinWidth: 0, + configuration: () => { + return { + minWidth: "280px", + minHeight: "280px", + }; + }, + }, + ], + }, + }, + "2": { + widgetId: "2", + leftColumn: 32, + rightColumn: 64, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 7, + type: "CURRENCY_INPUT_WIDGET", + widgetName: "Currency1", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + autoLayout: { + disabledPropsDefaults: { + labelPosition: LabelPosition.Top, + labelTextSize: "0.875rem", + }, + defaults: { + rows: 6.6, + }, + autoDimension: { + height: true, + }, + widgetSize: [ + { + viewportMinWidth: 0, + configuration: () => { + return { + minWidth: "120px", + }; + }, + }, + ], + disableResizeHandles: { + vertical: true, + }, + }, + }, + "3": { + widgetId: "3", + leftColumn: 64, + rightColumn: 96, + alignment: FlexLayerAlignment.End, + topRow: 0, + bottomRow: 7, + type: "CONTAINER_WIDGET", + widgetName: "Container1", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + }, + }; + jest + .spyOn(utils, "getWidgetMinMaxDimensionsInPixel") + // eslint-disable-next-line @typescript-eslint/no-unused-vars + .mockImplementation((widget: any, width: number) => { + if ( + ["DOCUMENT_VIEWER_WIDGET", "CONTAINER_WIDGET"].includes( + widget?.type, + ) + ) + return { + minWidth: 280, + minHeight: 280, + maxWidth: undefined, + maxHeight: undefined, + }; + if (widget?.type === "CURRENCY_INPUT_WIDGET") + return { + minWidth: 120, + minHeight: 40, + maxWidth: undefined, + maxHeight: undefined, + }; + return { + minWidth: undefined, + minHeight: undefined, + maxWidth: undefined, + maxHeight: undefined, + }; + }); + const layer: FlexLayer = { + children: [ + { id: "1", align: FlexLayerAlignment.Start }, + { id: "2", align: FlexLayerAlignment.Start }, + { id: "3", align: FlexLayerAlignment.End }, + ], + }; + const alignmentInfo = extractAlignmentInfo( + widgets, + layer, + false, + 640, + 10, + false, + ); + const res = placeWidgetsWithoutWrap( + widgets, + alignmentInfo.info, + 0, + false, + ); + + /** + * available columns: 64 + * column space: 10 + * # of fill widgets = 3 + * standard fill widget length (f) = 64 / 3 = 21.3333 + * min widths in descending order -> 28 (minWidth / columnSpace), 28, 12. + * + * In descending order of min widths: + * available columns: 64 + * 1st fill widget length (DocumentViewer) -> 28 > 21.3333 -> 28 + * available columns: 64 - 28 = 36 + * standard fill widget length (f) = 36 / 2 = 18 + * + * 2nd fill widget length (ContainerWidget) -> 28 > 18 -> 28 + * available columns: 36 - 28 = 8 + * standard fill widget length (f) = 8 / 1 = 8 + * + * 3rd fill widget length (CurrencyInput) -> 12 > 8 -> 12 + * + * => widgets will overflow the canvas. + * => min widths of each widget is respected. + */ + + // DocumentViewer + expect(res.widgets["1"].leftColumn).toEqual(0); + expect(res.widgets["1"].rightColumn).toEqual(28); + // CurrencyInput + expect(res.widgets["2"].leftColumn).toEqual(28); + expect(res.widgets["2"].rightColumn).toEqual(40); + // ContainerWidget + expect(res.widgets["3"].leftColumn).toEqual(40); + expect(res.widgets["3"].rightColumn).toEqual(68); + }); + + it("should allocate columns for fill widgets in descending order of their min width requirement - Part 2", () => { + const widgets = { + "1": { + widgetId: "1", + leftColumn: 0, + rightColumn: 21.3333, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 4, + type: "DOCUMENT_VIEWER_WIDGET", + widgetName: "", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + }, + "2": { + widgetId: "2", + leftColumn: 21.33333, + rightColumn: 42.66666, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 7, + type: "CURRENCY_INPUT_WIDGET", + widgetName: "Currency1", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + }, + "3": { + widgetId: "3", + leftColumn: 42.66666, + rightColumn: 63.99999, + alignment: FlexLayerAlignment.Start, + topRow: 0, + bottomRow: 7, + type: "CURRENCY_INPUT_WIDGET", + widgetName: "Currency2", + renderMode: RenderModes.CANVAS, + version: 1, + parentColumnSpace: 10, + parentRowSpace: 10, + isLoading: false, + responsiveBehavior: ResponsiveBehavior.Fill, + }, + }; + jest + .spyOn(utils, "getWidgetMinMaxDimensionsInPixel") + // eslint-disable-next-line @typescript-eslint/no-unused-vars + .mockImplementation((widget: any, width: number) => { + if ( + ["DOCUMENT_VIEWER_WIDGET", "CONTAINER_WIDGET"].includes( + widget?.type, + ) + ) + return { + minWidth: 280, + minHeight: 280, + maxWidth: undefined, + maxHeight: undefined, + }; + if (widget?.type === "CURRENCY_INPUT_WIDGET") + return { + minWidth: 120, + minHeight: 40, + maxWidth: undefined, + maxHeight: undefined, + }; + return { + minWidth: undefined, + minHeight: undefined, + maxWidth: undefined, + maxHeight: undefined, + }; + }); + const layer: FlexLayer = { + children: [ + { id: "1", align: FlexLayerAlignment.Start }, + { id: "2", align: FlexLayerAlignment.Start }, + { id: "3", align: FlexLayerAlignment.End }, + ], + }; + const alignmentInfo = extractAlignmentInfo( + widgets, + layer, + false, + 640, + 10, + false, + ); + const res = placeWidgetsWithoutWrap( + widgets, + alignmentInfo.info, + 0, + false, + ); + + /** + * total available columns = 64 + * # of fill widgets = 3 + * columnSpace = 10 + * standard fill widget length (f) = 64 / 3 = 21.333 + * min widths in descending order of columns -> 28 (280 / 10), 12, 12. + * + * In descending order of min widths: + * available columns: 64 + * 1st fill widget length (DocumentViewer) -> 28 > 21.3333 -> 28 + * available columns: 64 - 28 = 36 + * standard fill widget length (f) = 36 / 2 = 18 + * + * 2nd fill widget length (CurrencyInput) -> 12 < 18 -> 18 + * available columns: 36 - 18 = 18 + * standard fill widget length (f) = 18 / 1 = 18 + * + * 3rd fill widget length (CurrencyInput) -> 12 < 18 -> 180 + * + * => widgets don't overflow the canvas. + * => DocumentViewer gets more columns (28) to address its min width requirement. + * => rest of the space gets evenly distributed among the remaining fill widgets, as the it is larger than their min width requirement. + */ + + // DocumentViewer + expect(res.widgets["1"].leftColumn).toEqual(0); + expect(res.widgets["1"].rightColumn).toEqual(28); + // CurrencyInput1 + expect(res.widgets["2"].leftColumn).toEqual(28); + expect(res.widgets["2"].rightColumn).toEqual(46); + // CurrencyInput2 + expect(res.widgets["3"].leftColumn).toEqual(46); + expect(res.widgets["3"].rightColumn).toEqual(64); + }); }); describe("test placeWrappedWidgets method", () => { diff --git a/app/client/src/utils/autoLayout/positionUtils.ts b/app/client/src/utils/autoLayout/positionUtils.ts index 972d33c333..ac9a969790 100644 --- a/app/client/src/utils/autoLayout/positionUtils.ts +++ b/app/client/src/utils/autoLayout/positionUtils.ts @@ -401,7 +401,7 @@ export function extractAlignmentInfo( } } - const availableColumns: number = + let availableColumns: number = GridDefaults.DEFAULT_GRID_COLUMNS - startColumns - centerColumns - @@ -416,13 +416,11 @@ export function extractAlignmentInfo( .sort((a, b) => b.columns - a.columns) .forEach((each, index) => { const { child, columns } = each; - let width = fillWidgetLength; - // If the fill widget's minWidth is greater than the available space, then assign the widget's minWidth as its width. - if (columns > fillWidgetLength) { - width = columns; - fillWidgetLength = - (availableColumns - width) / (fillChildren.length - index - 1); - } + // If minWidth > availableColumns / # of fill widgets => assign columns based on minWidth and update fillLength for remaining fill widgets. + const width = columns > fillWidgetLength ? columns : fillWidgetLength; + availableColumns = Math.max(availableColumns - width, 0); + if (fillChildren.length - index - 1 > 0) + fillWidgetLength = availableColumns / (fillChildren.length - index - 1); if (child.align === FlexLayerAlignment.Start) { startColumns += width; const index = startChildren.findIndex(