Copilot commented on code in PR #34589:
URL: https://github.com/apache/superset/pull/34589#discussion_r2261049086
##########
superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx:
##########
@@ -116,6 +116,41 @@ export const showValueControl: ControlSetItem = {
},
};
+export const rotateValueControl: ControlSetItem = {
+ name: 'rotate_value',
+ config: {
+ type: 'SelectControl',
+ label: t('Rotate Value Label'),
+ default: 0,
+ freeForm: true,
+ clearable: false,
+ choices: [
+ [0, '0°'],
+ [45, '45°'],
+ [90, '90°'],
+ ],
+ renderTrigger: true,
+ description: t('Rotate the value labels visible on the chart'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.show_value?.value),
+ },
+};
+
+export const distanceValueControl: ControlSetItem = {
+ name: 'distance_value',
+ config: {
+ type: 'TextControl',
+ label: t('Value Label Distance'),
+ placeholder: t('Value Label Distance'),
+ default: 0,
+ isInt: true,
Review Comment:
The distance control accepts integers only, but distance values in chart
libraries often benefit from decimal precision for fine-tuning. Consider
removing the `isInt: true` constraint to allow decimal values.
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -199,6 +199,40 @@ function createCustomizeSection(
},
},
],
+ [
+ {
+ name: `rotate_value${controlSuffix}`,
+ config: {
+ type: 'SelectControl',
+ label: t('Rotate Value Label'),
+ renderTrigger: true,
+ default: 0,
+ description: t('Rotate the value label'),
+ choices: [
+ [0, t('0°')],
+ [45, t('45°')],
+ [90, t('90°')],
+ ],
+ freeForm: true,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.[`show_value${controlSuffix}`]?.value),
+ },
+ },
+ ],
+ [
+ {
+ name: `distance_value${controlSuffix}`,
+ config: {
+ type: 'TextControl',
+ label: t('Value Label Distance'),
+ default: 0,
+ renderTrigger: true,
+ description: t('Distance of the value label from the bar'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.[`show_value${controlSuffix}`].value),
Review Comment:
Missing optional chaining operator before `.value`. This should be
`controls?.[`show_value${controlSuffix}`]?.value` to prevent potential runtime
errors when the control doesn't exist.
```suggestion
Boolean(controls?.[`show_value${controlSuffix}`]?.value),
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -199,6 +199,40 @@ function createCustomizeSection(
},
},
],
+ [
+ {
+ name: `rotate_value${controlSuffix}`,
+ config: {
+ type: 'SelectControl',
+ label: t('Rotate Value Label'),
+ renderTrigger: true,
+ default: 0,
+ description: t('Rotate the value label'),
+ choices: [
+ [0, t('0°')],
+ [45, t('45°')],
+ [90, t('90°')],
+ ],
+ freeForm: true,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.[`show_value${controlSuffix}`]?.value),
+ },
+ },
+ ],
+ [
+ {
+ name: `distance_value${controlSuffix}`,
+ config: {
+ type: 'TextControl',
+ label: t('Value Label Distance'),
+ default: 0,
+ renderTrigger: true,
+ description: t('Distance of the value label from the bar'),
Review Comment:
The description mentions 'bar' but this is for timeseries charts which
typically use lines/areas. Consider using a more generic description like
'Distance of the value label from the data point'.
```suggestion
description: t('Distance of the value label from the data point'),
```
--
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]
