You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/04 07:24:53 UTC
[GitHub] [superset] villebro commented on a diff in pull request #19918: feat(plugin-chart-echarts): support horizontal bar chart
villebro commented on code in PR #19918:
URL: https://github.com/apache/superset/pull/19918#discussion_r864523160
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -318,57 +322,68 @@ export default function transformProps(
.map(entry => entry.name || '')
.concat(extractAnnotationLabels(annotationLayers, annotationData));
+ let xAxis: any = {
+ type: xAxisType,
+ name: xAxisTitle,
+ nameGap: convertInteger(xAxisTitleMargin),
+ nameLocation: 'middle',
+ axisLabel: {
+ hideOverlap: true,
+ formatter: xAxisFormatter,
+ rotate: xAxisLabelRotation,
+ },
+ minInterval:
+ xAxisType === 'time' && timeGrainSqla
+ ? TimeGrainToTimestamp[timeGrainSqla]
+ : 0,
+ };
+ let yAxis: any = {
+ ...defaultYAxis,
+ type: logAxis ? 'log' : 'value',
+ min,
+ max,
+ minorTick: { show: true },
+ minorSplitLine: { show: minorSplitLine },
+ axisLabel: { formatter },
+ scale: truncateYAxis,
+ name: yAxisTitle,
+ nameGap: convertInteger(yAxisTitleMargin),
+ nameLocation: yAxisTitlePosition === 'Left' ? 'middle' : 'end',
+ };
+
+ if (isHorizontal) {
+ const temp = xAxis;
+ xAxis = yAxis;
+ yAxis = temp;
+ }
+
const echartOptions: EChartsCoreOption = {
useUTC: true,
grid: {
...defaultGrid,
...padding,
},
- xAxis: {
- type: xAxisType,
- name: xAxisTitle,
- nameGap: convertInteger(xAxisTitleMargin),
- nameLocation: 'middle',
- axisLabel: {
- hideOverlap: true,
- formatter: xAxisFormatter,
- rotate: xAxisLabelRotation,
- },
- minInterval:
- xAxisType === 'time' && timeGrainSqla
- ? TimeGrainToTimestamp[timeGrainSqla]
- : 0,
- },
- yAxis: {
- ...defaultYAxis,
- type: logAxis ? 'log' : 'value',
- min,
- max,
- minorTick: { show: true },
- minorSplitLine: { show: minorSplitLine },
- axisLabel: { formatter },
- scale: truncateYAxis,
- name: yAxisTitle,
- nameGap: convertInteger(yAxisTitleMargin),
- nameLocation: yAxisTitlePosition === 'Left' ? 'middle' : 'end',
- },
+ xAxis,
+ yAxis,
tooltip: {
...defaultTooltip,
appendToBody: true,
trigger: richTooltip ? 'axis' : 'item',
formatter: (params: any) => {
+ const xIndex = isHorizontal ? 1 : 0;
+ const yIndex = isHorizontal ? 0 : 1;
Review Comment:
Slight simplification:
```suggestion
const [xIndex, yIndex] = isHorizontal ? [1, 0] : [0, 1];
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:
##########
@@ -87,6 +89,28 @@ const config: ControlPanelConfig = {
sections.advancedAnalyticsControls,
sections.annotationsAndLayersControls,
sections.forecastIntervalControls,
+ {
+ label: t('Chart Orientation'),
+ expanded: true,
+ controlSetRows: [
+ [
+ {
+ name: 'bar_orient',
Review Comment:
could we call this `orientation`? I'm sure transposing will be relevant for other chart types as well, so I'm not sure we need to restrict this to the bar chart. I would also move this control to the `src/controls.tsx` file to make it readily available to other charts in this plugin.
```suggestion
name: 'orientation',
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -318,57 +322,68 @@ export default function transformProps(
.map(entry => entry.name || '')
.concat(extractAnnotationLabels(annotationLayers, annotationData));
+ let xAxis: any = {
+ type: xAxisType,
+ name: xAxisTitle,
+ nameGap: convertInteger(xAxisTitleMargin),
+ nameLocation: 'middle',
+ axisLabel: {
+ hideOverlap: true,
+ formatter: xAxisFormatter,
+ rotate: xAxisLabelRotation,
+ },
+ minInterval:
+ xAxisType === 'time' && timeGrainSqla
+ ? TimeGrainToTimestamp[timeGrainSqla]
+ : 0,
+ };
+ let yAxis: any = {
+ ...defaultYAxis,
+ type: logAxis ? 'log' : 'value',
+ min,
+ max,
+ minorTick: { show: true },
+ minorSplitLine: { show: minorSplitLine },
+ axisLabel: { formatter },
+ scale: truncateYAxis,
+ name: yAxisTitle,
+ nameGap: convertInteger(yAxisTitleMargin),
+ nameLocation: yAxisTitlePosition === 'Left' ? 'middle' : 'end',
+ };
+
+ if (isHorizontal) {
+ const temp = xAxis;
+ xAxis = yAxis;
+ yAxis = temp;
Review Comment:
nit: I believe you could also do this:
```suggestion
[xAxis, yAxis] = [yAxis, xAxis]
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts:
##########
@@ -38,6 +38,11 @@ export enum EchartsTimeseriesContributionType {
Column = 'column',
}
+export enum EchartsOrientType {
Review Comment:
nit: I think `OrientationType` could be a simpler name for this (no need to prefix with `Echarts` as that's implied)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org