From 326b6174e9fdc4802f9883facd71a819603495da Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Wed, 5 Jun 2024 13:16:49 +0530 Subject: [PATCH] fix: drop of bbs inside container causing reflow (#33947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Current way of dropping building blocks already knows the widget id where it is going to be pasted. `getNewPositionsBasedOnMousePositions` did not take that into account and used to recalculate the canvas and container widgets. - This led to reflow and BBs getting reflowed further down(along with its container) This PR adds a fix to that ensuring no extra calculation and subsequently no further reflow. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Widget, @tag.Templates, @tag.IDE" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: ba9c447fc662d2c1386b4caadd6dc04355c60082 > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling when pasting building block widgets. - Enhanced logic for determining container widget when pasting into a specific widget. - **Tests** - Updated test descriptions for better readability. - Added parameter handling in test cases for widget pasting scenarios. --- .../BuildingBlockAdditionSagas.ts | 6 ++-- .../PasteBuildingBlockWidgetSaga.test.ts | 5 +-- .../src/sagas/PasteWidgetUtils/index.ts | 33 ++++++++++++++++--- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts b/app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts index 99a9dc2c6f..b0f7f70cf4 100644 --- a/app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts +++ b/app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts @@ -284,6 +284,7 @@ export function* loadBuildingBlocksIntoApplication( } } } catch (error) { + log.error("Error loading building blocks into application", error); yield put({ type: WidgetReduxActionTypes.WIDGET_SINGLE_DELETE, payload: { @@ -420,8 +421,6 @@ export function* pasteBuildingBlockWidgetsSaga( const mainCanvasWidth: number = yield select(getCanvasWidth); try { - const isThereACollision = false; - if ( // to avoid invoking old way of copied widgets implementaion !Array.isArray(copiedWidgetGroups) || @@ -458,6 +457,7 @@ export function* pasteBuildingBlockWidgetsSaga( topMostWidget.topRow, leftMostWidget.leftColumn, { gridPosition }, + pastingIntoWidgetId, ); for (const widgetGroup of copiedWidgetGroups) { //This is required when you cut the widget as CanvasWidgetState doesn't have the widget anymore @@ -507,7 +507,7 @@ export function* pasteBuildingBlockWidgetsSaga( nextAvailableRow, newPastingPositionMap, true, - isThereACollision, + false, false, ); diff --git a/app/client/src/sagas/BuildingBlockSagas/PasteBuildingBlockWidgetSaga.test.ts b/app/client/src/sagas/BuildingBlockSagas/PasteBuildingBlockWidgetSaga.test.ts index 3ab78ffda2..1149afd662 100644 --- a/app/client/src/sagas/BuildingBlockSagas/PasteBuildingBlockWidgetSaga.test.ts +++ b/app/client/src/sagas/BuildingBlockSagas/PasteBuildingBlockWidgetSaga.test.ts @@ -43,7 +43,7 @@ type GeneratorType = Generator< describe("pasteBuildingBlockWidgetsSaga", () => { const copiedWidgetsResponse = { widgets: copiedWidgets, flexLayers }; - it("should handle pasting into a valid parent widget", () => { + it("1. should handle pasting into a valid parent widget", () => { const generator: GeneratorType = pasteBuildingBlockWidgetsSaga( gridPosition, parentWidgetId, @@ -101,6 +101,7 @@ describe("pasteBuildingBlockWidgetsSaga", () => { boundaryWidgets.topMostWidget.topRow, boundaryWidgets.leftMostWidget.leftColumn, { gridPosition }, + parentWidgetId, ), ); @@ -136,7 +137,7 @@ describe("pasteBuildingBlockWidgetsSaga", () => { expect(result.done).toBe(true); }); - it("should handle errors gracefully", () => { + it("2. should handle errors gracefully", () => { const generator: GeneratorType = pasteBuildingBlockWidgetsSaga( { left: 0, top: 0 }, "testParentId", diff --git a/app/client/src/sagas/PasteWidgetUtils/index.ts b/app/client/src/sagas/PasteWidgetUtils/index.ts index c8551e329f..14deaf608d 100644 --- a/app/client/src/sagas/PasteWidgetUtils/index.ts +++ b/app/client/src/sagas/PasteWidgetUtils/index.ts @@ -1,10 +1,13 @@ import type { EitherMouseLocationORGridPosition } from "constants/WidgetConstants"; -import { GridDefaults } from "constants/WidgetConstants"; +import { + GridDefaults, + MAIN_CONTAINER_WIDGET_ID, +} from "constants/WidgetConstants"; import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer"; import { call, select } from "redux-saga/effects"; import { getContainerWidgetSpacesSelector } from "selectors/editorSelectors"; import type { WidgetProps } from "widgets/BaseWidget"; -import { getWidgets } from "../selectors"; +import { getWidget, getWidgets } from "../selectors"; import type { FlattenedWidgetProps } from "WidgetProvider/constants"; import type { WidgetSpace } from "constants/CanvasEditorConstants"; @@ -48,6 +51,7 @@ export /** * @param copiedTopMostRow top row of the top most copied widget * @param copiedLeftMostColumn left column of the left most copied widget * @param gridPosition left and top canvas grid position values + * @param pastingIntoWidgetId - The ID of the widget where the new widgets should be pasted into. * @returns */ const getNewPositions = function* ( @@ -56,6 +60,7 @@ const getNewPositions = function* ( copiedTopMostRow: number, copiedLeftMostColumn: number, whereToPasteWidget: EitherMouseLocationORGridPosition, + pastingIntoWidgetId?: string, ) { const selectedWidgetIDs: string[] = yield select(getSelectedWidgets); const canvasWidgets: CanvasWidgetsReduxState = yield select(getWidgets); @@ -107,6 +112,7 @@ const getNewPositions = function* ( copiedTopMostRow, copiedLeftMostColumn, whereToPasteWidget, + pastingIntoWidgetId, ); return newPastingPositionDetails; }; @@ -124,6 +130,7 @@ const getNewPositions = function* ( * @param copiedTopMostRow top row of the top most copied widget * @param copiedLeftMostColumn left column of the left most copied widget * @param gridPosition left and top canvas grid position values + * @param pastingIntoWidgetId - The ID of the widget where the new widgets should be pasted into. If this is known we don't need to calculate it. In addition, the container widget and dom queries will be different. * @returns */ function* getNewPositionsBasedOnMousePositions( @@ -134,15 +141,33 @@ function* getNewPositionsBasedOnMousePositions( copiedTopMostRow: number, copiedLeftMostColumn: number, whereToPasteWidget: EitherMouseLocationORGridPosition, + pastingIntoWidgetId?: string, ) { - let { canvasDOM, canvasId, containerWidget } = - getDefaultCanvas(canvasWidgets); + let { + canvasDOM, + canvasId, + containerWidget, + }: { + canvasDOM: Element | null | undefined; + canvasId: string | undefined; + containerWidget: FlattenedWidgetProps | undefined; + } = getDefaultCanvas(canvasWidgets); //if the selected widget is a layout widget then change the pasting canvas. if (selectedWidgets.length === 1 && isDropTarget(selectedWidgets[0].type)) { containerWidget = selectedWidgets[0]; ({ canvasDOM, canvasId } = getCanvasIdForContainer(containerWidget)); } + if (pastingIntoWidgetId && pastingIntoWidgetId !== MAIN_CONTAINER_WIDGET_ID) { + // For building block we already know the widget where we want to paste the new widgets + // no need to calculate it + const containerWidgetId = getContainerIdForCanvas(pastingIntoWidgetId); + if (!containerWidgetId) return {}; + containerWidget = yield select(getWidget, containerWidgetId); + ({ canvasDOM, canvasId } = getCanvasIdForContainer( + containerWidget as WidgetProps, + )); + } if (!canvasDOM || !containerWidget || !canvasId) return {};