korbit-ai[bot] commented on code in PR #33146:
URL: https://github.com/apache/superset/pull/33146#discussion_r2046346517
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -104,6 +199,20 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'x_axis_label_distance',
+ config: {
+ type: 'TextControl',
+ label: t('X Axis Label Distance'),
+ description: t('Distance of the label from X axis (in pixels)'),
+ renderTrigger: true,
+ default: '25',
+ isInt: true,
+ // validators: [v => !Number.isNaN(v) && v >= 0],
Review Comment:
### Remove or implement commented validators <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
There are commented-out validators for both x_axis_label_distance and
y_axis_label_distance controls that should either be implemented or removed.
###### Why this matters
Commented-out code creates confusion about whether the validation is
intended to be used and adds unnecessary noise to the codebase.
###### Suggested change ∙ *Feature Preview*
Either remove the commented validators or implement them properly:
```typescript
validators: [v => !Number.isNaN(v) && v >= 0],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11909f6-e26b-40c8-bdf7-2dae3ea6b119/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11909f6-e26b-40c8-bdf7-2dae3ea6b119?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11909f6-e26b-40c8-bdf7-2dae3ea6b119?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11909f6-e26b-40c8-bdf7-2dae3ea6b119?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11909f6-e26b-40c8-bdf7-2dae3ea6b119)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:efdd2929-3f92-4bc3-9205-c45b0700ee66 -->
[](efdd2929-3f92-4bc3-9205-c45b0700ee66)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -39,6 +40,48 @@ const config: ControlPanelConfig = {
['metric'],
['adhoc_filters'],
['row_limit'],
+ [
+ {
+ name: 'seriesOrderByColumn',
+ config: {
+ type: 'SelectControl',
+ label: t('Order Series By Column'),
+ description: t(
+ 'Column to use for ordering the waterfall series with columns
not in the chart',
+ ),
+ mapStateToProps: state => ({
+ choices: [
+ [null, t('None')],
+ ...(state.datasource?.columns || []).map(col => [
+ col.column_name,
+ col.column_name,
+ ]),
+ ],
+ }),
+ default: null,
+ renderTrigger: true,
+ },
+ },
+ ],
+ [
+ {
+ name: 'seriesOrderDirection',
+ config: {
+ type: 'SelectControl',
+ label: t('Order Direction'),
+ choices: [
+ [null, t('None')],
+ ['ASC', t('Ascending')],
+ ['DESC', t('Descending')],
+ ],
+ default: null,
+ renderTrigger: true,
Review Comment:
### Order Direction Control Not Dependent on Column Selection
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The seriesOrderDirection control allows selecting an order direction even
when no seriesOrderByColumn is selected, which can lead to confusing behavior.
###### Why this matters
Users might set an order direction without a column to order by, resulting
in no effect and potentially confusing the user about why their sorting
configuration isn't working.
###### Suggested change ∙ *Feature Preview*
Add visibility condition to only show the order direction when a column is
selected:
```typescript
name: 'seriesOrderDirection',
config: {
type: 'SelectControl',
label: t('Order Direction'),
choices: [
[null, t('None')],
['ASC', t('Ascending')],
['DESC', t('Descending')],
],
default: null,
renderTrigger: true,
visibility: ({ controls }) =>
!!controls.seriesOrderByColumn.value,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69eefb0a-8dcb-418d-8cc7-fbe56fce6153/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69eefb0a-8dcb-418d-8cc7-fbe56fce6153?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69eefb0a-8dcb-418d-8cc7-fbe56fce6153?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69eefb0a-8dcb-418d-8cc7-fbe56fce6153?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69eefb0a-8dcb-418d-8cc7-fbe56fce6153)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4bba72dd-fa1a-4e07-8dbe-c78ba6bff083 -->
[](4bba72dd-fa1a-4e07-8dbe-c78ba6bff083)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -104,6 +199,20 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'x_axis_label_distance',
+ config: {
+ type: 'TextControl',
+ label: t('X Axis Label Distance'),
+ description: t('Distance of the label from X axis (in pixels)'),
+ renderTrigger: true,
+ default: '25',
+ isInt: true,
Review Comment:
### Type inconsistency in default value <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The default value for axis label distances is specified as a string '25'
while isInt: true indicates it should be a number.
###### Why this matters
This type inconsistency between the default value and the expected type can
lead to confusion and potential type coercion issues.
###### Suggested change ∙ *Feature Preview*
Change the default value to be a number:
```typescript
default: 25,
isInt: true,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0957cfe-f4e8-45ae-8b37-2fe00fa28fe4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0957cfe-f4e8-45ae-8b37-2fe00fa28fe4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0957cfe-f4e8-45ae-8b37-2fe00fa28fe4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0957cfe-f4e8-45ae-8b37-2fe00fa28fe4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0957cfe-f4e8-45ae-8b37-2fe00fa28fe4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:09c5d9bf-ffdf-4218-bac3-cba410f1d9bb -->
[](09c5d9bf-ffdf-4218-bac3-cba410f1d9bb)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -37,13 +161,351 @@ export default function EchartsWaterfall(
},
};
+ const getSubtotalOptions = (options: EChartsCoreOption) => {
+ if (!useFirstValueAsSubtotal) return options;
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const processedSeries = ((options.series as any[]) || []).map(series => {
+ const newData = series.data.map((dataPoint: any, index: number) => {
+ if (index !== 0) return dataPoint;
+
+ const isTransparent =
+ dataPoint?.itemStyle?.color &&
+ dataPoint.itemStyle.color === 'transparent';
+
+ if (isTransparent) return dataPoint;
+
+ if (dataPoint.value === '-') return dataPoint;
+
+ const updatedColor = `rgba(${totalColor.r}, ${totalColor.g},
${totalColor.b}, ${totalColor.a})`;
+ return {
+ ...dataPoint,
+ itemStyle: {
+ ...dataPoint.itemStyle,
+ color: updatedColor,
+ borderColor: updatedColor,
+ },
+ };
+ });
+
+ return {
+ ...series,
+ data: newData,
+ };
+ });
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: processedSeries,
+ };
+ };
+
+ const getShowTotalOptions = (options: EChartsCoreOption) => {
+ if (showTotal) return options;
+
+ const totalsIndex =
+ ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || [];
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ].filter((_, index) => !totalsIndex.includes(index));
+
+ const filteredSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: series.data.filter(
+ (_: any, index: number) => !totalsIndex.includes(index),
+ ),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: filteredSeries,
+ };
+ };
+
+ const getSortedOptions = (options: EChartsCoreOption) => {
+ if (sortXAxis === 'none') return options;
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const sortedData = [...xAxisData];
+
+ sortedData.sort((a, b) => {
+ if (typeof a === 'number' && typeof b === 'number') {
+ return sortXAxis === 'asc' ? a - b : b - a;
+ }
+ const aStr = String(a);
+ const bStr = String(b);
+ return sortXAxis === 'asc'
+ ? aStr.localeCompare(bStr)
+ : bStr.localeCompare(aStr);
+ });
+
+ const indexMap = new Map(xAxisData.map((val, index) => [val, index]));
+
+ const sortedSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: sortedData.map(value => {
+ const index = indexMap.get(value);
+ return index !== undefined ? (series as any).data[index] : null;
+ }),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: sortedData,
+ },
+ series: sortedSeries,
+ };
+ };
+
+ const getFlippedOptions = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') return options;
+
+ return {
+ ...options,
+ xAxis: {
+ ...((options.yAxis as any) || {}),
+ type: 'value',
+ axisLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light3,
+ width: 1,
+ },
+ },
+ splitLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light2,
+ width: 1,
+ type: 'solid',
+ },
+ },
+ name: (options.yAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ yAxis: {
+ ...((options.xAxis as any) || {}),
+ type: 'category',
+ axisLine: { show: true },
+ data: [...(options.xAxis as any).data].reverse(),
+ name: (options.xAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ series: Array.isArray(options.series)
+ ? options.series.map((series: any) => ({
+ ...series,
+ encode: {
+ x: series.encode?.y,
+ y: series.encode?.x,
+ },
+ data: [...series.data].reverse(),
+ label: {
+ ...(series.label || {}),
+ position: series.name === 'Decrease' ? 'left' : 'right',
+ },
+ }))
+ : [],
+ };
+ };
+
+ const getFormattedAxisOptions = (options: EChartsCoreOption) => {
+ const { xTicksLayout, xTicksWrapLength } = props.formData;
+
+ // If no formatting needed, return original options
+ if (!boldTotal && !boldSubTotal && xTicksLayout !== 'flat') {
+ return options;
+ }
+
+ // Get total indices for bold formatting
+ const totalsIndex = boldTotal
+ ? ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || []
+ : [];
+
+ const formatText = (value: string, index: number) => {
+ // Handle bold formatting first
+ let formattedValue = value;
+
+ if (orientation === 'vertical') {
+ if (index === 0 && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ } else {
+ const axisData = (options.yAxis as { data?: any[] })?.data || [];
+ const isLast = index === axisData.length - 1;
+ if (isLast && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ }
+
+ // get the width of xAxis to calculate the maxCharsPerLine
+ const getAxisRange = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') {
+ const xAxis = options.xAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area width accounting for grid margins
+ const availableWidth = width - (grid?.left || 0) - (grid?.right ||
0);
+
+ if (xAxis?.type === 'value') {
+ const range = xAxis.max - xAxis.min;
+ return Math.min(range, availableWidth);
+ }
+ if (xAxis?.type === 'category') {
+ const categories = xAxis.data?.length || 1;
+ // Calculate space per category
+ return availableWidth / categories;
+ }
+ } else {
+ const yAxis = options.yAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area height accounting for grid margins
+ const availableHeight =
+ height - (grid?.top || 0) - (grid?.bottom || 0);
+
+ if (yAxis?.type === 'value') {
+ const range = yAxis.max - yAxis.min;
+ return Math.min(range, availableHeight);
+ }
+ if (yAxis?.type === 'category') {
+ const categories = yAxis.data?.length || 1;
+ // Calculate space per category
+ return availableHeight / categories;
+ }
+ }
+
+ // Fallback to a reasonable default
+ return orientation === 'vertical' ? width * 0.8 : height * 0.8;
+ };
+
+ // Handle text wrapping if needed
+ const maxWidth = getAxisRange(options); // chart width
+ const maxCharsPerLine = Math.floor(maxWidth / xTicksWrapLength); //
Approx chars per line
Review Comment:
### Inaccurate Text Wrapping Calculation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The text wrapping calculation assumes a fixed character width without
accounting for variable-width fonts or special characters, which could lead to
improper wrapping of axis labels.
###### Why this matters
This simplistic approach to calculating text width can result in
inconsistent or incorrect line breaks, potentially causing text overflow or
poor layout in the chart's axis labels.
###### Suggested change ∙ *Feature Preview*
Use a more robust approach to calculate text width:
```typescript
const getTextWidth = (text: string) => {
const canvas = document.createElement('canvas');
const context = canvas.getContext('2d');
if (!context) return 0;
context.font =
chartRef.current?.getEchartInstance().getOption()?.textStyle?.fontFamily ||
'sans-serif';
return context.measureText(text).width;
};
const calculateWrappedText = (text: string, maxWidth: number) => {
const words = text.split(' ');
let line = '';
let wrappedText = '';
words.forEach(word => {
const testLine = line + word + ' ';
if (getTextWidth(testLine) > maxWidth) {
wrappedText += line.trim() + '\n';
line = word + ' ';
} else {
line = testLine;
}
});
return wrappedText + line.trim();
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0babf288-44d6-45e7-87ea-804ea1f002bb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0babf288-44d6-45e7-87ea-804ea1f002bb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0babf288-44d6-45e7-87ea-804ea1f002bb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0babf288-44d6-45e7-87ea-804ea1f002bb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0babf288-44d6-45e7-87ea-804ea1f002bb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8c391d30-23c8-4391-a78d-628c1d576cce -->
[](8c391d30-23c8-4391-a78d-628c1d576cce)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/index.ts:
##########
@@ -17,17 +17,16 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ChartMetadata, ChartPlugin, t } from '@superset-ui/core';
+import { Behavior, ChartMetadata, ChartPlugin, t } from '@superset-ui/core';
import buildQuery from './buildQuery';
import controlPanel from './controlPanel';
-import transformProps from './transformProps';
+import transformProps from './transformProps-copy';
import thumbnail from './images/thumbnail.png';
import example1 from './images/example1.png';
import example2 from './images/example2.png';
import example3 from './images/example3.png';
-import { EchartsWaterfallChartProps, EchartsWaterfallFormData } from './types';
+import { EchartsWaterfallChartProps, EchartsWaterfallFormData } from
'./types-copy';
Review Comment:
### Suspicious file suffix in types import <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Similar to above, the types import path includes '-copy' suffix suggesting
temporary file usage.
###### Why this matters
Temporary file naming in production code can lead to maintenance confusion
and versioning issues.
###### Suggested change ∙ *Feature Preview*
```typescript
import { EchartsWaterfallChartProps, EchartsWaterfallFormData } from
'./types';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/79a9a675-581f-40ec-b83c-a0213a2bac7c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/79a9a675-581f-40ec-b83c-a0213a2bac7c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/79a9a675-581f-40ec-b83c-a0213a2bac7c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/79a9a675-581f-40ec-b83c-a0213a2bac7c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/79a9a675-581f-40ec-b83c-a0213a2bac7c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f385883e-ad09-4ed4-b493-4f49ab6a22ba -->
[](f385883e-ad09-4ed4-b493-4f49ab6a22ba)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -16,16 +16,140 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { EChartsCoreOption } from 'echarts/core';
+import { useTheme } from '@superset-ui/core';
+import React, { useRef } from 'react';
import Echart from '../components/Echart';
-import { WaterfallChartTransformedProps } from './types';
+import { WaterfallChartTransformedProps } from './types-copy';
import { EventHandlers } from '../types';
export default function EchartsWaterfall(
props: WaterfallChartTransformedProps,
) {
- const { height, width, echartOptions, refs, onLegendStateChanged } = props;
+ const {
+ height,
+ width,
+ echartOptions,
+ setDataMask,
+ onContextMenu,
+ refs,
+ onLegendStateChanged,
+ formData: {
+ sortXAxis,
+ orientation,
+ showTotal,
+ useFirstValueAsSubtotal,
+ totalColor,
+ xAxisLabelDistance,
+ yAxisLabelDistance,
+ boldTotal,
+ boldSubTotal,
+ },
+ emitCrossFilters,
+ } = props;
+
+ const theme = useTheme();
+ const chartRef = useRef<any>(null);
const eventHandlers: EventHandlers = {
+ click: params => {
Review Comment:
### Missing documentation for complex click handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The click event handler has complex business logic related to filtering and
visual state management, but lacks documentation explaining its purpose and
behavior.
###### Why this matters
Without clear documentation of this handler's behavior, future maintainers
will have difficulty understanding the filtering logic and its interaction with
the chart's visual state.
###### Suggested change ∙ *Feature Preview*
const eventHandlers: EventHandlers = {
/**
* Handles click events on chart elements to manage cross-filtering.
* When an element is clicked:
* - Applies filter if element isn't currently filtered
* - Clears filter if element is currently filtered
* - Updates visual state to highlight/unhighlight elements
* - Ignores clicks on 'Total' column
*/
click: params => {
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a10bc2ef-52ff-4f34-892e-3bba1ac60bf2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a10bc2ef-52ff-4f34-892e-3bba1ac60bf2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a10bc2ef-52ff-4f34-892e-3bba1ac60bf2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a10bc2ef-52ff-4f34-892e-3bba1ac60bf2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a10bc2ef-52ff-4f34-892e-3bba1ac60bf2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5ad15ae9-42d4-4b1f-9b11-681a4364fba6 -->
[](5ad15ae9-42d4-4b1f-9b11-681a4364fba6)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -37,13 +161,351 @@ export default function EchartsWaterfall(
},
};
+ const getSubtotalOptions = (options: EChartsCoreOption) => {
+ if (!useFirstValueAsSubtotal) return options;
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const processedSeries = ((options.series as any[]) || []).map(series => {
+ const newData = series.data.map((dataPoint: any, index: number) => {
+ if (index !== 0) return dataPoint;
+
+ const isTransparent =
+ dataPoint?.itemStyle?.color &&
+ dataPoint.itemStyle.color === 'transparent';
+
+ if (isTransparent) return dataPoint;
+
+ if (dataPoint.value === '-') return dataPoint;
+
+ const updatedColor = `rgba(${totalColor.r}, ${totalColor.g},
${totalColor.b}, ${totalColor.a})`;
+ return {
+ ...dataPoint,
+ itemStyle: {
+ ...dataPoint.itemStyle,
+ color: updatedColor,
+ borderColor: updatedColor,
+ },
+ };
+ });
+
+ return {
+ ...series,
+ data: newData,
+ };
+ });
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: processedSeries,
+ };
+ };
+
+ const getShowTotalOptions = (options: EChartsCoreOption) => {
+ if (showTotal) return options;
+
+ const totalsIndex =
+ ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || [];
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ].filter((_, index) => !totalsIndex.includes(index));
+
+ const filteredSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: series.data.filter(
+ (_: any, index: number) => !totalsIndex.includes(index),
+ ),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: filteredSeries,
+ };
+ };
+
+ const getSortedOptions = (options: EChartsCoreOption) => {
+ if (sortXAxis === 'none') return options;
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const sortedData = [...xAxisData];
+
+ sortedData.sort((a, b) => {
+ if (typeof a === 'number' && typeof b === 'number') {
+ return sortXAxis === 'asc' ? a - b : b - a;
+ }
+ const aStr = String(a);
+ const bStr = String(b);
+ return sortXAxis === 'asc'
+ ? aStr.localeCompare(bStr)
+ : bStr.localeCompare(aStr);
+ });
Review Comment:
### Inconsistent Mixed-Type Data Sorting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sorting logic mixes numeric and string comparisons without handling
mixed-type data correctly, which could lead to unexpected sorting results when
the data contains both numbers and strings.
###### Why this matters
When the dataset contains both numbers and strings, the current
implementation might produce inconsistent sorting orders, affecting the
visualization's clarity and user understanding.
###### Suggested change ∙ *Feature Preview*
Implement a consistent sorting strategy that handles mixed types uniformly:
```typescript
sortedData.sort((a, b) => {
const aNum = Number(a);
const bNum = Number(b);
const areNumbers = !Number.isNaN(aNum) && !Number.isNaN(bNum);
if (areNumbers) {
return sortXAxis === 'asc' ? aNum - bNum : bNum - aNum;
}
// If not both numbers, treat as strings
const aStr = String(a);
const bStr = String(b);
return sortXAxis === 'asc'
? aStr.localeCompare(bStr)
: bStr.localeCompare(aStr);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f6e896b7-835b-413c-9ce8-c27fc392e2f1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f6e896b7-835b-413c-9ce8-c27fc392e2f1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f6e896b7-835b-413c-9ce8-c27fc392e2f1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f6e896b7-835b-413c-9ce8-c27fc392e2f1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f6e896b7-835b-413c-9ce8-c27fc392e2f1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0a294b1d-b4fe-4d27-ac47-e65eeacdf45a -->
[](0a294b1d-b4fe-4d27-ac47-e65eeacdf45a)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -16,16 +16,140 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { EChartsCoreOption } from 'echarts/core';
+import { useTheme } from '@superset-ui/core';
+import React, { useRef } from 'react';
import Echart from '../components/Echart';
-import { WaterfallChartTransformedProps } from './types';
+import { WaterfallChartTransformedProps } from './types-copy';
import { EventHandlers } from '../types';
export default function EchartsWaterfall(
props: WaterfallChartTransformedProps,
) {
- const { height, width, echartOptions, refs, onLegendStateChanged } = props;
+ const {
+ height,
+ width,
+ echartOptions,
+ setDataMask,
+ onContextMenu,
+ refs,
+ onLegendStateChanged,
+ formData: {
+ sortXAxis,
+ orientation,
+ showTotal,
+ useFirstValueAsSubtotal,
+ totalColor,
+ xAxisLabelDistance,
+ yAxisLabelDistance,
+ boldTotal,
+ boldSubTotal,
+ },
+ emitCrossFilters,
+ } = props;
+
+ const theme = useTheme();
+ const chartRef = useRef<any>(null);
const eventHandlers: EventHandlers = {
+ click: params => {
+ if (!setDataMask || !emitCrossFilters) return;
+
+ const { name: value } = params;
+ const xAxisColumn = props.formData.xAxis;
+
+ // Don't filter on Total column
+ if (value === 'Total') return;
+
+ const isCurrentValue = props.filterState?.value === value;
+
+ if (isCurrentValue) {
+ // Clear the filter and visual state
+ setDataMask({
+ extraFormData: {},
+ filterState: {
+ value: null,
+ },
+ });
+
+ if (chartRef.current) {
+ const series = echartOptions.series as any[];
+ const updatedSeries = series.map(s => ({
+ ...s,
+ itemStyle: {
+ ...s.itemStyle,
+ opacity: 1,
+ },
+ }));
+
+ if (orientation === 'vertical') {
+ chartRef.current.getEchartInstance().setOption({
+ series: updatedSeries,
+ });
+ } else {
+ chartRef.current.getEchartInstance().setOption({
+ series: updatedSeries.map(s => ({
+ ...s,
+ data: [...s.data].reverse(),
+ })),
+ });
+ }
+ }
+ return;
+ }
+
+ // Set new filter
+ setDataMask({
+ extraFormData: {
+ filters: [
+ {
+ col: xAxisColumn,
+ op: '==',
+ val: value,
+ },
+ ],
+ },
+ filterState: {
+ value,
+ },
+ });
+
+ if (chartRef.current) {
+ const series = echartOptions.series as any[];
+ const xAxisLabel = echartOptions.xAxis as { data: (string | number)[]
};
+ // get index of value in xAxisLabel
+ const valueIndex = (xAxisLabel?.data || []).indexOf(value);
+
+ const updatedSeries = series.map(s => ({
+ ...s,
+ data: s.data.map((d: any, idx: number) => ({
+ ...d,
+ itemStyle: {
+ ...d.itemStyle,
+ opacity: !Number.isNaN(d.value) && idx === valueIndex ? 1 : 0.3,
+ },
+ })),
+ }));
Review Comment:
### Scattered series transformation logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The series transformation logic is repeated in multiple places with similar
patterns, making the code harder to follow.
###### Why this matters
Similar transformation patterns scattered throughout the code increase
cognitive load and make maintenance more difficult.
###### Suggested change ∙ *Feature Preview*
Extract common series transformation logic into a utility function:
```typescript
const updateSeriesOpacity = (series: any[], valueIndex: number | null) =>
series.map(s => ({
...s,
data: s.data.map((d: any, idx: number) => ({
...d,
itemStyle: {
...d.itemStyle,
opacity: valueIndex === null ? 1 : (!Number.isNaN(d.value) && idx ===
valueIndex ? 1 : 0.3),
},
})),
}));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/56a0579f-6bc7-4dc4-a0ab-8b307240bb82/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/56a0579f-6bc7-4dc4-a0ab-8b307240bb82?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/56a0579f-6bc7-4dc4-a0ab-8b307240bb82?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/56a0579f-6bc7-4dc4-a0ab-8b307240bb82?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/56a0579f-6bc7-4dc4-a0ab-8b307240bb82)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d314ddb5-9ccd-4ac8-8337-b43cd6f53f46 -->
[](d314ddb5-9ccd-4ac8-8337-b43cd6f53f46)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -37,13 +161,351 @@ export default function EchartsWaterfall(
},
};
+ const getSubtotalOptions = (options: EChartsCoreOption) => {
+ if (!useFirstValueAsSubtotal) return options;
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const processedSeries = ((options.series as any[]) || []).map(series => {
+ const newData = series.data.map((dataPoint: any, index: number) => {
+ if (index !== 0) return dataPoint;
+
+ const isTransparent =
+ dataPoint?.itemStyle?.color &&
+ dataPoint.itemStyle.color === 'transparent';
+
+ if (isTransparent) return dataPoint;
+
+ if (dataPoint.value === '-') return dataPoint;
+
+ const updatedColor = `rgba(${totalColor.r}, ${totalColor.g},
${totalColor.b}, ${totalColor.a})`;
+ return {
+ ...dataPoint,
+ itemStyle: {
+ ...dataPoint.itemStyle,
+ color: updatedColor,
+ borderColor: updatedColor,
+ },
+ };
+ });
+
+ return {
+ ...series,
+ data: newData,
+ };
+ });
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: processedSeries,
+ };
+ };
+
+ const getShowTotalOptions = (options: EChartsCoreOption) => {
+ if (showTotal) return options;
+
+ const totalsIndex =
+ ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || [];
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ].filter((_, index) => !totalsIndex.includes(index));
+
+ const filteredSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: series.data.filter(
+ (_: any, index: number) => !totalsIndex.includes(index),
+ ),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: filteredSeries,
+ };
+ };
+
+ const getSortedOptions = (options: EChartsCoreOption) => {
+ if (sortXAxis === 'none') return options;
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const sortedData = [...xAxisData];
+
+ sortedData.sort((a, b) => {
+ if (typeof a === 'number' && typeof b === 'number') {
+ return sortXAxis === 'asc' ? a - b : b - a;
+ }
+ const aStr = String(a);
+ const bStr = String(b);
+ return sortXAxis === 'asc'
+ ? aStr.localeCompare(bStr)
+ : bStr.localeCompare(aStr);
+ });
+
+ const indexMap = new Map(xAxisData.map((val, index) => [val, index]));
+
+ const sortedSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: sortedData.map(value => {
+ const index = indexMap.get(value);
+ return index !== undefined ? (series as any).data[index] : null;
+ }),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: sortedData,
+ },
+ series: sortedSeries,
+ };
+ };
+
+ const getFlippedOptions = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') return options;
+
+ return {
+ ...options,
+ xAxis: {
+ ...((options.yAxis as any) || {}),
+ type: 'value',
+ axisLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light3,
+ width: 1,
+ },
+ },
+ splitLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light2,
+ width: 1,
+ type: 'solid',
+ },
+ },
+ name: (options.yAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ yAxis: {
+ ...((options.xAxis as any) || {}),
+ type: 'category',
+ axisLine: { show: true },
+ data: [...(options.xAxis as any).data].reverse(),
+ name: (options.xAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ series: Array.isArray(options.series)
+ ? options.series.map((series: any) => ({
+ ...series,
+ encode: {
+ x: series.encode?.y,
+ y: series.encode?.x,
+ },
+ data: [...series.data].reverse(),
+ label: {
+ ...(series.label || {}),
+ position: series.name === 'Decrease' ? 'left' : 'right',
+ },
+ }))
+ : [],
+ };
+ };
+
+ const getFormattedAxisOptions = (options: EChartsCoreOption) => {
+ const { xTicksLayout, xTicksWrapLength } = props.formData;
+
+ // If no formatting needed, return original options
+ if (!boldTotal && !boldSubTotal && xTicksLayout !== 'flat') {
+ return options;
+ }
+
+ // Get total indices for bold formatting
+ const totalsIndex = boldTotal
+ ? ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || []
+ : [];
+
+ const formatText = (value: string, index: number) => {
+ // Handle bold formatting first
+ let formattedValue = value;
+
+ if (orientation === 'vertical') {
+ if (index === 0 && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ } else {
+ const axisData = (options.yAxis as { data?: any[] })?.data || [];
+ const isLast = index === axisData.length - 1;
+ if (isLast && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ }
+
+ // get the width of xAxis to calculate the maxCharsPerLine
+ const getAxisRange = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') {
+ const xAxis = options.xAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area width accounting for grid margins
+ const availableWidth = width - (grid?.left || 0) - (grid?.right ||
0);
+
+ if (xAxis?.type === 'value') {
+ const range = xAxis.max - xAxis.min;
+ return Math.min(range, availableWidth);
+ }
+ if (xAxis?.type === 'category') {
+ const categories = xAxis.data?.length || 1;
+ // Calculate space per category
+ return availableWidth / categories;
+ }
+ } else {
+ const yAxis = options.yAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area height accounting for grid margins
+ const availableHeight =
+ height - (grid?.top || 0) - (grid?.bottom || 0);
+
+ if (yAxis?.type === 'value') {
+ const range = yAxis.max - yAxis.min;
+ return Math.min(range, availableHeight);
+ }
+ if (yAxis?.type === 'category') {
+ const categories = yAxis.data?.length || 1;
+ // Calculate space per category
+ return availableHeight / categories;
+ }
+ }
+
+ // Fallback to a reasonable default
+ return orientation === 'vertical' ? width * 0.8 : height * 0.8;
+ };
+
+ // Handle text wrapping if needed
+ const maxWidth = getAxisRange(options); // chart width
+ const maxCharsPerLine = Math.floor(maxWidth / xTicksWrapLength); //
Approx chars per line
+
+ const words = formattedValue.split(' ');
+ let line = '';
+ let wrappedText = '';
+
+ words.forEach(word => {
+ if ((line + word).length > maxCharsPerLine) {
+ wrappedText += `${line.trim()}\n`;
+ line = `${word} `;
+ } else {
+ line += `${word} `;
+ }
+ });
+
+ wrappedText += line.trim();
+
+ return wrappedText;
+ };
+
+ if (orientation === 'vertical') {
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ axisLabel: {
+ ...(options.xAxis as any)?.axisLabel,
+ formatter: formatText,
+ overflow: 'break',
+ rich: {
+ ...(options.xAxis as any)?.axisLabel?.rich,
+ subtotal: boldSubTotal ? { fontWeight: 'bold' } : undefined,
+ total: boldTotal ? { fontWeight: 'bold' } : undefined,
+ },
+ },
+ },
+ };
+ }
+
+ return {
+ ...options,
+ yAxis: {
+ ...(options.yAxis as any),
+ axisLabel: {
+ ...(options.yAxis as any)?.axisLabel,
+ formatter: formatText,
+ overflow: 'break',
+ rich: {
+ ...(options.yAxis as any)?.axisLabel?.rich,
+ subtotal: boldSubTotal ? { fontWeight: 'bold' } : undefined,
+ total: boldTotal ? { fontWeight: 'bold' } : undefined,
+ },
+ },
+ },
+ };
+ };
+
+ const getLabelDistanceOptions = (options: EChartsCoreOption) => {
+ if (Number.isNaN(Number(xAxisLabelDistance))) {
+ console.error('xAxisLabelDistance should be a number');
Review Comment:
### Inadequate Error Handling for Invalid Label Distance <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Error handling in getLabelDistanceOptions only logs to console and continues
execution with potentially invalid data.
###### Why this matters
Silent failure with console logging can lead to unexpected chart behavior
since invalid values are still used in chart rendering.
###### Suggested change ∙ *Feature Preview*
```typescript
const getLabelDistanceOptions = (options: EChartsCoreOption) => {
if (Number.isNaN(Number(xAxisLabelDistance))) {
throw new Error('xAxisLabelDistance must be a valid number');
}
if (Number.isNaN(Number(yAxisLabelDistance))) {
throw new Error('yAxisLabelDistance must be a valid number');
}
return {
...options,
xAxis: {
...(options.xAxis as any),
nameGap: Number(xAxisLabelDistance),
},
yAxis: {
...(options.yAxis as any),
nameGap: Number(yAxisLabelDistance),
},
};
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d220c37-7b62-4491-86f5-f46984c786e0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d220c37-7b62-4491-86f5-f46984c786e0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d220c37-7b62-4491-86f5-f46984c786e0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d220c37-7b62-4491-86f5-f46984c786e0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6d220c37-7b62-4491-86f5-f46984c786e0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a2a9ea6c-3388-461d-993b-93870db96dff -->
[](a2a9ea6c-3388-461d-993b-93870db96dff)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -37,13 +161,351 @@ export default function EchartsWaterfall(
},
};
+ const getSubtotalOptions = (options: EChartsCoreOption) => {
+ if (!useFirstValueAsSubtotal) return options;
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const processedSeries = ((options.series as any[]) || []).map(series => {
+ const newData = series.data.map((dataPoint: any, index: number) => {
+ if (index !== 0) return dataPoint;
+
+ const isTransparent =
+ dataPoint?.itemStyle?.color &&
+ dataPoint.itemStyle.color === 'transparent';
+
+ if (isTransparent) return dataPoint;
+
+ if (dataPoint.value === '-') return dataPoint;
+
+ const updatedColor = `rgba(${totalColor.r}, ${totalColor.g},
${totalColor.b}, ${totalColor.a})`;
+ return {
+ ...dataPoint,
+ itemStyle: {
+ ...dataPoint.itemStyle,
+ color: updatedColor,
+ borderColor: updatedColor,
+ },
+ };
+ });
+
+ return {
+ ...series,
+ data: newData,
+ };
+ });
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: processedSeries,
+ };
+ };
+
+ const getShowTotalOptions = (options: EChartsCoreOption) => {
+ if (showTotal) return options;
+
+ const totalsIndex =
+ ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || [];
+
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ].filter((_, index) => !totalsIndex.includes(index));
+
+ const filteredSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: series.data.filter(
+ (_: any, index: number) => !totalsIndex.includes(index),
+ ),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: xAxisData,
+ },
+ series: filteredSeries,
+ };
+ };
+
+ const getSortedOptions = (options: EChartsCoreOption) => {
+ if (sortXAxis === 'none') return options;
+ const xAxisData = [
+ ...((options.xAxis as { data: (string | number)[] }).data || []),
+ ];
+
+ const sortedData = [...xAxisData];
+
+ sortedData.sort((a, b) => {
+ if (typeof a === 'number' && typeof b === 'number') {
+ return sortXAxis === 'asc' ? a - b : b - a;
+ }
+ const aStr = String(a);
+ const bStr = String(b);
+ return sortXAxis === 'asc'
+ ? aStr.localeCompare(bStr)
+ : bStr.localeCompare(aStr);
+ });
+
+ const indexMap = new Map(xAxisData.map((val, index) => [val, index]));
+
+ const sortedSeries = ((options.series as any[]) || []).map(series => ({
+ ...series,
+ data: sortedData.map(value => {
+ const index = indexMap.get(value);
+ return index !== undefined ? (series as any).data[index] : null;
+ }),
+ }));
+
+ return {
+ ...options,
+ xAxis: {
+ ...(options.xAxis as any),
+ data: sortedData,
+ },
+ series: sortedSeries,
+ };
+ };
+
+ const getFlippedOptions = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') return options;
+
+ return {
+ ...options,
+ xAxis: {
+ ...((options.yAxis as any) || {}),
+ type: 'value',
+ axisLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light3,
+ width: 1,
+ },
+ },
+ splitLine: {
+ show: true,
+ lineStyle: {
+ color: theme.colors.grayscale.light2,
+ width: 1,
+ type: 'solid',
+ },
+ },
+ name: (options.yAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ yAxis: {
+ ...((options.xAxis as any) || {}),
+ type: 'category',
+ axisLine: { show: true },
+ data: [...(options.xAxis as any).data].reverse(),
+ name: (options.xAxis as any)?.name || '',
+ nameLocation: 'middle',
+ },
+ series: Array.isArray(options.series)
+ ? options.series.map((series: any) => ({
+ ...series,
+ encode: {
+ x: series.encode?.y,
+ y: series.encode?.x,
+ },
+ data: [...series.data].reverse(),
+ label: {
+ ...(series.label || {}),
+ position: series.name === 'Decrease' ? 'left' : 'right',
+ },
+ }))
+ : [],
+ };
+ };
+
+ const getFormattedAxisOptions = (options: EChartsCoreOption) => {
+ const { xTicksLayout, xTicksWrapLength } = props.formData;
+
+ // If no formatting needed, return original options
+ if (!boldTotal && !boldSubTotal && xTicksLayout !== 'flat') {
+ return options;
+ }
+
+ // Get total indices for bold formatting
+ const totalsIndex = boldTotal
+ ? ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || []
+ : [];
+
+ const formatText = (value: string, index: number) => {
+ // Handle bold formatting first
+ let formattedValue = value;
+
+ if (orientation === 'vertical') {
+ if (index === 0 && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ } else {
+ const axisData = (options.yAxis as { data?: any[] })?.data || [];
+ const isLast = index === axisData.length - 1;
+ if (isLast && boldSubTotal) {
+ formattedValue = `{subtotal|${value}}`;
+ } else if (totalsIndex.includes(index) && boldTotal) {
+ formattedValue = `{total|${value}}`;
+ }
+ }
+
+ // get the width of xAxis to calculate the maxCharsPerLine
+ const getAxisRange = (options: EChartsCoreOption) => {
+ if (orientation === 'vertical') {
+ const xAxis = options.xAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area width accounting for grid margins
+ const availableWidth = width - (grid?.left || 0) - (grid?.right ||
0);
+
+ if (xAxis?.type === 'value') {
+ const range = xAxis.max - xAxis.min;
+ return Math.min(range, availableWidth);
+ }
+ if (xAxis?.type === 'category') {
+ const categories = xAxis.data?.length || 1;
+ // Calculate space per category
+ return availableWidth / categories;
+ }
+ } else {
+ const yAxis = options.yAxis as any;
+ const grid = options.grid as any;
+
+ // Get actual chart area height accounting for grid margins
+ const availableHeight =
+ height - (grid?.top || 0) - (grid?.bottom || 0);
+
+ if (yAxis?.type === 'value') {
+ const range = yAxis.max - yAxis.min;
+ return Math.min(range, availableHeight);
+ }
+ if (yAxis?.type === 'category') {
+ const categories = yAxis.data?.length || 1;
+ // Calculate space per category
+ return availableHeight / categories;
+ }
+ }
+
+ // Fallback to a reasonable default
+ return orientation === 'vertical' ? width * 0.8 : height * 0.8;
+ };
Review Comment:
### Deeply nested complex function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getAxisRange function is nested within getFormattedAxisOptions making it
hard to read and maintain. It's a complex function that handles both vertical
and horizontal orientations with deep nesting.
###### Why this matters
Deeply nested functions with complex branching make the code harder to
follow and maintain. It also makes it difficult to test the logic independently.
###### Suggested change ∙ *Feature Preview*
Extract getAxisRange as a standalone function at the component level:
```typescript
const getAxisRange = (options: EChartsCoreOption, width: number, height:
number, orientation: string) => {
const calculateValueRange = (axis: any, availableSpace: number) => {
if (axis?.type === 'value') {
return Math.min(axis.max - axis.min, availableSpace);
}
if (axis?.type === 'category') {
return availableSpace / (axis.data?.length || 1);
}
return availableSpace * 0.8;
};
const grid = options.grid as any;
if (orientation === 'vertical') {
const availableWidth = width - (grid?.left || 0) - (grid?.right || 0);
return calculateValueRange(options.xAxis, availableWidth);
}
const availableHeight = height - (grid?.top || 0) - (grid?.bottom || 0);
return calculateValueRange(options.yAxis, availableHeight);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80b0d8e9-e7e7-47e0-9230-a0b3b8d1157b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80b0d8e9-e7e7-47e0-9230-a0b3b8d1157b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80b0d8e9-e7e7-47e0-9230-a0b3b8d1157b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80b0d8e9-e7e7-47e0-9230-a0b3b8d1157b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/80b0d8e9-e7e7-47e0-9230-a0b3b8d1157b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:049bc31b-876c-488b-a00b-0cabbdfc4a29 -->
[](049bc31b-876c-488b-a00b-0cabbdfc4a29)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
