Skip to content

Commit

Permalink
fix: Improve conditional rendering in ChartWidget (#36806)
Browse files Browse the repository at this point in the history
## Description
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.


Fixes #36213
_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.Chart"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11288506543>
> Commit: 09b7635
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11288506543&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Chart`
> Spec:
> <hr>Fri, 11 Oct 2024 08:17:45 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced the `ChartWidget` component for improved control flow and
readability.
- Introduced a streamlined rendering process for different chart states
(loading, empty, error).
- Added an optional `onDataPointClick` property to the
`ChartWidgetProps` interface.

- **Improvements**
	- Modularized rendering logic for better maintainability.
- Added a comprehensive set of unit tests for the `ChartWidget`
component to ensure consistent rendering behavior across various states.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
rahulbarwal authored Oct 11, 2024
1 parent 7644b67 commit ef5a253
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 34 deletions.
125 changes: 125 additions & 0 deletions app/client/src/widgets/ChartWidget/widget/ChartsRendered.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
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;

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,
};
const errors: WidgetError[] = [
{
name: "error",
type: "configuration",
message: "We have a error",
},
];

describe("loading state", () => {
it("isLoading: true", () => {
chartWidget = new ChartWidget({ ...defaultProps, isLoading: true });

const view = chartWidget.getWidgetView();

expect(view.props.children.props.isLoading).toBe(true);
});

it("isLoading: true + errors", () => {
chartWidget = new ChartWidget({
...defaultProps,
isLoading: true,
errors,
});

const view = chartWidget.getWidgetView();

expect(view.props.children.props.isLoading).toBe(true);
});
it("isLoading: true + emptyChartData", () => {
chartWidget = new ChartWidget({
...defaultProps,
isLoading: true,
chartData: {},
});

const view = chartWidget.getWidgetView();

expect(view.props.children.props.isLoading).toBe(true);
});
});

it("renders error state", () => {
chartWidget = new ChartWidget({
...defaultProps,
errors,
});

const view = chartWidget.getWidgetView();

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.type).toEqual(EmptyChartData);
});

it("renders chart with data", () => {
chartWidget = new ChartWidget(defaultProps);
const view = chartWidget.getWidgetView();

expect(view.props.children.props.chartData).toEqual({
seriesID1: seriesData1,
seriesID2: seriesData2,
});
});
});
76 changes: 42 additions & 34 deletions app/client/src/widgets/ChartWidget/widget/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -226,42 +226,50 @@ class ChartWidget extends BaseWidget<ChartWidgetProps, WidgetState> {
getWidgetView() {
const errors = syntaxErrorsFromProps(this.props);

if (errors.length == 0) {
if (emptyChartData(this.props)) {
return <EmptyChartData />;
} else {
return (
<Suspense fallback={<Skeleton />}>
<ChartComponent
allowScroll={this.props.allowScroll}
borderRadius={this.props.borderRadius}
boxShadow={this.props.boxShadow}
chartData={this.props.chartData}
chartName={this.props.chartName}
chartType={this.props.chartType}
customEChartConfig={this.props.customEChartConfig}
customFusionChartConfig={this.props.customFusionChartConfig}
dimensions={this.props}
fontFamily={ChartWidget.fontFamily}
hasOnDataPointClick={Boolean(this.props.onDataPointClick)}
isLoading={this.props.isLoading}
isVisible={this.props.isVisible}
key={this.props.widgetId}
labelOrientation={this.props.labelOrientation}
onDataPointClick={this.onDataPointClick}
primaryColor={this.props.accentColor ?? Colors.ROYAL_BLUE_2}
setAdaptiveYMin={this.props.setAdaptiveYMin}
showDataPointLabel={this.props.showDataPointLabel}
widgetId={this.props.widgetId}
xAxisName={this.props.xAxisName}
yAxisName={this.props.yAxisName}
/>
</Suspense>
);
}
} else {
if (this.props.isLoading) {
return this.renderChartWithData();
}

if (errors.length > 0) {
return <ChartErrorComponent error={errors[0]} />;
}

if (emptyChartData(this.props)) {
return <EmptyChartData />;
}

return this.renderChartWithData();
}

renderChartWithData() {
return (
<Suspense fallback={<Skeleton />}>
<ChartComponent
allowScroll={this.props.allowScroll}
borderRadius={this.props.borderRadius}
boxShadow={this.props.boxShadow}
chartData={this.props.chartData}
chartName={this.props.chartName}
chartType={this.props.chartType}
customEChartConfig={this.props.customEChartConfig}
customFusionChartConfig={this.props.customFusionChartConfig}
dimensions={this.props}
fontFamily={ChartWidget.fontFamily}
hasOnDataPointClick={Boolean(this.props.onDataPointClick)}
isLoading={this.props.isLoading}
isVisible={this.props.isVisible}
key={this.props.widgetId}
labelOrientation={this.props.labelOrientation}
onDataPointClick={this.onDataPointClick}
primaryColor={this.props.accentColor ?? Colors.ROYAL_BLUE_2}
setAdaptiveYMin={this.props.setAdaptiveYMin}
showDataPointLabel={this.props.showDataPointLabel}
widgetId={this.props.widgetId}
xAxisName={this.props.xAxisName}
yAxisName={this.props.yAxisName}
/>
</Suspense>
);
}
}

Expand Down

0 comments on commit ef5a253

Please sign in to comment.