rusackas commented on code in PR #38695:
URL: https://github.com/apache/superset/pull/38695#discussion_r3869141167
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -748,7 +757,7 @@ export function getPadding(
left:
yAxisTitlePosition === 'Left'
? TIMESERIES_CONSTANTS.gridOffsetLeft +
- (Number(yAxisTitleMargin) || 0)
+ (Number(yAxisTitleMargin) || 0)
: TIMESERIES_CONSTANTS.gridOffsetLeft,
Review Comment:
This function isn't touched by this PR, the diff only adds label-position
logic elsewhere in the file. Out of scope here.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -180,6 +180,8 @@ export default function transformProps(
showLegend,
showValue,
showValueB,
+ labelPosition,
+ labelPositionB,
onlyTotal,
Review Comment:
Looks like this is covered now, `test('threads labelPosition and
labelPositionB to series A and B', ...)` in `transformProps.test.ts` asserts
both `seriesA.label.position` and `seriesB.label.position`.
##########
superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx:
##########
@@ -140,6 +140,29 @@ export const showValueControl: ControlSetItem = {
},
};
+export const labelPositionControl: ControlSetItem = {
+ name: 'label_position',
+ config: {
+ type: 'SelectControl',
+ freeForm: false,
+ label: t('Label Position'),
+ choices: [
+ ['auto', t('Auto')],
+ ['top', t('Top')],
+ ['inside', t('Inside')],
+ ['bottom', t('Bottom')],
+ ['left', t('Left')],
+ ['right', t('Right')],
+ ],
+ default: 'auto',
+ renderTrigger: true,
+ description: t('Position of the data label relative to the bar segment'),
Review Comment:
Fixed, description now just says "relative to the data point".
##########
superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx:
##########
@@ -140,6 +140,29 @@ export const showValueControl: ControlSetItem = {
},
};
+export const labelPositionControl: ControlSetItem = {
+ name: 'label_position',
+ config: {
+ type: 'SelectControl',
+ freeForm: false,
+ label: t('Label Position'),
+ choices: [
+ ['auto', t('Auto')],
+ ['top', t('Top')],
+ ['inside', t('Inside')],
+ ['bottom', t('Bottom')],
+ ['left', t('Left')],
+ ['right', t('Right')],
+ ],
+ default: 'auto',
+ renderTrigger: true,
+ description: t('Position of the data label relative to the bar segment'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.show_value?.value) ||
+ Boolean(controls?.show_valueB?.value),
Review Comment:
Fixed, this checks `show_value` now.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -201,6 +201,31 @@ function createCustomizeSection(
},
},
],
+ [
+ {
+ name: `label_position${controlSuffix}`,
+ config: {
+ type: 'SelectControl',
+ freeForm: false,
+ label: t('Label Position'),
+ choices: [
+ ['auto', t('Auto')],
+ ['top', t('Top')],
+ ['inside', t('Inside')],
+ ['bottom', t('Bottom')],
+ ['left', t('Left')],
+ ['right', t('Right')],
+ ],
+ default: 'auto',
+ renderTrigger: true,
+ description: t(
+ 'Position of the data label relative to the bar segment',
+ ),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.[`show_value${controlSuffix}`]?.value),
+ },
Review Comment:
Same fix as the controls.tsx thread, description's generalized now.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -416,7 +428,12 @@ export function transformSeries(
symbolSize: markerSize,
label: {
show: !!showValue,
- position: isHorizontal ? 'right' : 'top',
+ position: (labelPosition === 'auto' || !labelPosition
+ ? isHorizontal
+ ? 'right'
+ : 'top'
+ : labelPosition) as any,
Review Comment:
This is resolved too, the cast is `as LabelPositionEnum` now, no `any` left
here.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:
##########
@@ -84,6 +84,28 @@ export type EchartsMixedTimeseriesFormData = QueryFormData &
{
seriesTypeB: EchartsTimeseriesSeriesType;
showValue: boolean;
showValueB: boolean;
+ labelPosition?:
+ | 'auto'
+ | 'top'
+ | 'bottom'
+ | 'left'
+ | 'right'
+ | 'inside'
+ | 'insideTop'
+ | 'insideBottom'
+ | 'insideLeft'
+ | 'insideRight';
+ labelPositionB?:
+ | 'auto'
+ | 'top'
+ | 'bottom'
+ | 'left'
+ | 'right'
+ | 'inside'
+ | 'insideTop'
+ | 'insideBottom'
+ | 'insideLeft'
+ | 'insideRight';
Review Comment:
labelPosition/labelPositionB are typed as `LabelPositionEnum | 'auto'` now,
matches the control's own enum instead of a hand-rolled union.
--
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]
