From 12dd9af969118b45be7b1c3db66b0714b1921df9 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Thu, 10 Oct 2024 18:04:32 +0530 Subject: [PATCH 1/8] refactor: Improve conditional rendering in ChartWidget Simplify the conditional rendering logic in the ChartWidget component by separating the cases for an empty chart and loading state. This improves readability and maintainability of the code. --- .../src/widgets/ChartWidget/widget/index.tsx | 78 ++++++++++--------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/app/client/src/widgets/ChartWidget/widget/index.tsx b/app/client/src/widgets/ChartWidget/widget/index.tsx index db04a5f907dc..5484edfaf3a2 100644 --- a/app/client/src/widgets/ChartWidget/widget/index.tsx +++ b/app/client/src/widgets/ChartWidget/widget/index.tsx @@ -225,43 +225,51 @@ class ChartWidget extends BaseWidget { getWidgetView() { const errors = syntaxErrorsFromProps(this.props); + const isLoading = this.props.isLoading; + const hasErrors = errors.length > 0; + const isEmptyChart = emptyChartData(this.props); - if (errors.length == 0) { - if (emptyChartData(this.props)) { - return ; - } else { - return ( - }> - - - ); - } - } else { - return ; + if (isEmptyChart && !isLoading) { + return ; + } + + if (isLoading || !hasErrors) { + return this.renderChartWithData(); } + + // There are errors and chart is not loading + return ; + } + + renderChartWithData() { + return ( + }> + + + ); } } From 2b7813a408f734fa00681d64095d48d1e7f0f76f Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 11:34:44 +0530 Subject: [PATCH 2/8] refactor: Improve conditional rendering in ChartWidget --- .../src/widgets/ChartWidget/widget/index.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/client/src/widgets/ChartWidget/widget/index.tsx b/app/client/src/widgets/ChartWidget/widget/index.tsx index 5484edfaf3a2..9b95ed665182 100644 --- a/app/client/src/widgets/ChartWidget/widget/index.tsx +++ b/app/client/src/widgets/ChartWidget/widget/index.tsx @@ -225,20 +225,20 @@ class ChartWidget extends BaseWidget { getWidgetView() { const errors = syntaxErrorsFromProps(this.props); - const isLoading = this.props.isLoading; - const hasErrors = errors.length > 0; - const isEmptyChart = emptyChartData(this.props); - if (isEmptyChart && !isLoading) { - return ; + if (this.props.isLoading) { + return this.renderChartWithData(); } - if (isLoading || !hasErrors) { - return this.renderChartWithData(); + if (errors.length > 0) { + return ; + } + + if (emptyChartData(this.props)) { + return ; } - // There are errors and chart is not loading - return ; + return this.renderChartWithData(); } renderChartWithData() { From 572795951229d1aa6708d059e46d2f51470cfe21 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 11:35:40 +0530 Subject: [PATCH 3/8] Adds ChartWidget rendering unit tests --- .../widget/ChartsRendered.test.tsx | 89 ++++++ .../ChartsRendered.test.tsx.snap | 253 ++++++++++++++++++ 2 files changed, 342 insertions(+) create mode 100644 app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx create mode 100644 app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap diff --git a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx new file mode 100644 index 000000000000..528c5ec01e61 --- /dev/null +++ b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx @@ -0,0 +1,89 @@ +import { RenderModes } from "constants/WidgetConstants"; +import ChartWidget, { type ChartWidgetProps } from "."; +import { LabelOrientation, type ChartData } from "../constants"; + +describe("ChartWidget getWidgetView", () => { + let chartWidget: ChartWidget; + + const seriesData1: ChartData = { + seriesName: "series1", + data: [{ x: "x1", y: 1 }], + color: "series1color", + }; + const seriesData2: ChartData = { + seriesName: "series2", + data: [{ x: "x1", y: 2 }], + color: "series2color", + }; + const defaultProps: ChartWidgetProps = { + allowScroll: true, + showDataPointLabel: true, + chartData: { + seriesID1: seriesData1, + seriesID2: seriesData2, + }, + chartName: "chart name", + type: "CHART_WIDGET", + chartType: "AREA_CHART", + customEChartConfig: {}, + customFusionChartConfig: { type: "type", dataSource: undefined }, + hasOnDataPointClick: true, + isVisible: true, + isLoading: false, + setAdaptiveYMin: false, + labelOrientation: LabelOrientation.AUTO, + onDataPointClick: "", + widgetId: "widgetID", + xAxisName: "xaxisname", + yAxisName: "yaxisname", + borderRadius: "1", + boxShadow: "1", + primaryColor: "primarycolor", + fontFamily: "fontfamily", + dimensions: { componentWidth: 11, componentHeight: 11 }, + parentColumnSpace: 1, + parentRowSpace: 1, + topRow: 0, + bottomRow: 0, + leftColumn: 0, + rightColumn: 0, + widgetName: "widgetName", + version: 1, + renderMode: RenderModes.CANVAS, + }; + + it("renders loading state", () => { + chartWidget = new ChartWidget({ ...defaultProps, isLoading: true }); + + const view = chartWidget.getWidgetView(); + + expect(view).toMatchSnapshot(); + }); + + it("renders error state", () => { + chartWidget = new ChartWidget({ + ...defaultProps, + errors: [ + { name: "error", type: "configuration", message: "We have a error" }, + ], + }); + + const view = chartWidget.getWidgetView(); + + expect(view).toMatchSnapshot(); + }); + + it("renders empty chart data state", () => { + chartWidget = new ChartWidget({ ...defaultProps, chartData: {} }); + const view = chartWidget.getWidgetView(); + + expect(view).toMatchSnapshot(); + }); + + it("renders chart with data", () => { + chartWidget = new ChartWidget(defaultProps); + const view = chartWidget.getWidgetView(); + + expect(view).toMatchSnapshot(); + }); +}); diff --git a/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap b/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap new file mode 100644 index 000000000000..75416c60a95d --- /dev/null +++ b/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap @@ -0,0 +1,253 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`ChartWidget getWidgetView renders chart with data 1`] = ` +} +> + + +`; + +exports[`ChartWidget getWidgetView renders empty chart data state 1`] = ``; + +exports[`ChartWidget getWidgetView renders error state 1`] = ` + +`; + +exports[`ChartWidget getWidgetView renders loading state 1`] = ` +} +> + + +`; From b1db1b65c8f314f4de9ecd4934543d390cfbfd2b Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 11:54:39 +0530 Subject: [PATCH 4/8] refactor: Improve conditional rendering in ChartWidget unit tests --- .../ChartWidget/widget/ChartsRendered.test.tsx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx index 528c5ec01e61..1f030c1d5ff0 100644 --- a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx +++ b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx @@ -1,6 +1,7 @@ import { RenderModes } from "constants/WidgetConstants"; import ChartWidget, { type ChartWidgetProps } from "."; import { LabelOrientation, type ChartData } from "../constants"; +import { EmptyChartData } from "../component/EmptyChartData"; describe("ChartWidget getWidgetView", () => { let chartWidget: ChartWidget; @@ -57,7 +58,7 @@ describe("ChartWidget getWidgetView", () => { const view = chartWidget.getWidgetView(); - expect(view).toMatchSnapshot(); + expect(view.props.children.props.isLoading).toBe(true); }); it("renders error state", () => { @@ -70,20 +71,23 @@ describe("ChartWidget getWidgetView", () => { const view = chartWidget.getWidgetView(); - expect(view).toMatchSnapshot(); + expect(view.props.error.message).toBe("We have a error"); }); it("renders empty chart data state", () => { chartWidget = new ChartWidget({ ...defaultProps, chartData: {} }); const view = chartWidget.getWidgetView(); - expect(view).toMatchSnapshot(); + expect(view.type).toEqual(EmptyChartData); }); it("renders chart with data", () => { chartWidget = new ChartWidget(defaultProps); const view = chartWidget.getWidgetView(); - expect(view).toMatchSnapshot(); + expect(view.props.children.props.chartData).toEqual({ + seriesID1: seriesData1, + seriesID2: seriesData2, + }); }); }); From fc812fd8062ed8255e508ec793289a3e50021b2d Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 11:55:04 +0530 Subject: [PATCH 5/8] refactor: Remove unused snapshot file for ChartWidget unit tests --- .../ChartsRendered.test.tsx.snap | 253 ------------------ 1 file changed, 253 deletions(-) delete mode 100644 app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap diff --git a/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap b/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap deleted file mode 100644 index 75416c60a95d..000000000000 --- a/app/client/src/widgets/ChartWidget/widget/__snapshots__/ChartsRendered.test.tsx.snap +++ /dev/null @@ -1,253 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`ChartWidget getWidgetView renders chart with data 1`] = ` -} -> - - -`; - -exports[`ChartWidget getWidgetView renders empty chart data state 1`] = ``; - -exports[`ChartWidget getWidgetView renders error state 1`] = ` - -`; - -exports[`ChartWidget getWidgetView renders loading state 1`] = ` -} -> - - -`; From d76fd568569e3a9a1617da750938da05b4a0cd24 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 11:56:30 +0530 Subject: [PATCH 6/8] Removes unnecessary lines --- .../src/widgets/ChartWidget/widget/ChartsRendered.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx index 1f030c1d5ff0..e1127731a9ca 100644 --- a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx +++ b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx @@ -76,6 +76,7 @@ describe("ChartWidget getWidgetView", () => { it("renders empty chart data state", () => { chartWidget = new ChartWidget({ ...defaultProps, chartData: {} }); + const view = chartWidget.getWidgetView(); expect(view.type).toEqual(EmptyChartData); From fa8f9c38f5ca4c1e90d0bea9b105fbd814a4c4cb Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 13:16:44 +0530 Subject: [PATCH 7/8] Refactor ChartWidget unit tests and improve conditional rendering This commit refactors the ChartWidget unit tests by removing unnecessary lines and improving the conditional rendering. It also adds rendering unit tests for the ChartWidget. Additionally, it introduces a new type WidgetError for handling errors in the widget. --- .../widget/ChartsRendered.test.tsx | 46 ++++++++++++++++--- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx index e1127731a9ca..51665cc1006e 100644 --- a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx +++ b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx @@ -2,6 +2,7 @@ import { RenderModes } from "constants/WidgetConstants"; import ChartWidget, { type ChartWidgetProps } from "."; import { LabelOrientation, type ChartData } from "../constants"; import { EmptyChartData } from "../component/EmptyChartData"; +import type { WidgetError } from "widgets/BaseWidget"; describe("ChartWidget getWidgetView", () => { let chartWidget: ChartWidget; @@ -52,21 +53,52 @@ describe("ChartWidget getWidgetView", () => { version: 1, renderMode: RenderModes.CANVAS, }; + const errors: WidgetError[] = [ + { + name: "error", + type: "configuration", + message: "We have a error", + }, + ]; - it("renders loading state", () => { - chartWidget = new ChartWidget({ ...defaultProps, isLoading: true }); + describe("loading state", () => { + it("isLoading: true", () => { + chartWidget = new ChartWidget({ ...defaultProps, isLoading: true }); - const view = chartWidget.getWidgetView(); + const view = chartWidget.getWidgetView(); + + expect(view.props.children.props.isLoading).toBe(true); + }); + + it("isLoading: true + errors", () => { + chartWidget = new ChartWidget({ + ...defaultProps, + isLoading: true, + errors, + }); - expect(view.props.children.props.isLoading).toBe(true); + const view = chartWidget.getWidgetView(); + + expect(view.props.children.props.isLoading).toBe(true); + }); + it("isLoading: true + emptyChartData", () => { + chartWidget = new ChartWidget({ + ...defaultProps, + isLoading: true, + errors, + chartData: {}, + }); + + const view = chartWidget.getWidgetView(); + + expect(view.props.children.props.isLoading).toBe(true); + }); }); it("renders error state", () => { chartWidget = new ChartWidget({ ...defaultProps, - errors: [ - { name: "error", type: "configuration", message: "We have a error" }, - ], + errors, }); const view = chartWidget.getWidgetView(); From 09b7635c04bbf72b12a6461deafc0789f7a04ea7 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Fri, 11 Oct 2024 13:23:03 +0530 Subject: [PATCH 8/8] Refactor ChartWidget unit tests: Remove unnecessary lines and improve conditional rendering --- .../src/widgets/ChartWidget/widget/ChartsRendered.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx index 51665cc1006e..b569208e7f47 100644 --- a/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx +++ b/app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx @@ -85,7 +85,6 @@ describe("ChartWidget getWidgetView", () => { chartWidget = new ChartWidget({ ...defaultProps, isLoading: true, - errors, chartData: {}, });