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/11/13 09:28:33 UTC
[GitHub] [superset] mayurnewase opened a new pull request, #22107: [WIP] feat: migrate bubble char to echarts
mayurnewase opened a new pull request, #22107:
URL: https://github.com/apache/superset/pull/22107
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Migrates D3 bubble chart to Echart's scatter.
bubble chart has extra controls than scatter, so might be worth replacing it first then combine scatter and bubble chart types later.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
[GitHub] [superset] villebro commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034894502
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,207 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+ getMetricLabel,
+ NumberFormatter,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType, Refs } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+import { getDefaultTooltip } from '../utils/tooltip';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+ xAxisFormatter: NumberFormatter,
+ yAxisFormatter: NumberFormatter,
+ tooltipSizeFormatter: NumberFormatter,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${xAxisFormatter(params.data[0])} <br/>
+ ${yAxisLabel}: ${yAxisFormatter(params.data[1])} <br/>
+ ${sizeLabel}: ${tooltipSizeFormatter(params.data[2])}`;
+}
+
+export default function transformProps(chartProps: EchartsBubbleChartProps) {
+ const { height, width, hooks, queriesData, formData, inContextMenu } =
+ chartProps;
+
+ const { data = [] } = queriesData[0];
+ const {
+ x,
+ y,
+ size,
+ entity,
+ maxBubbleSize,
+ colorScheme,
+ series: bubbleSeries,
+ xAxisLabel: bubbleXAxisTitle,
+ yAxisLabel: bubbleYAxisTitle,
+ xAxisFormat,
+ yAxisFormat,
+ yAxisBounds,
+ logXAxis,
+ logYAxis,
+ xAxisTitleMargin,
+ yAxisTitleMargin,
+ truncateYAxis,
+ xAxisLabelRotation,
+ yAxisLabelRotation,
+ tooltipSizeFormat,
+ opacity,
+ }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData };
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+
+ const legends: string[] = [];
+ const series: ScatterSeriesOption[] = [];
+
+ const xAxisLabel: string = getMetricLabel(x);
+ const yAxisLabel: string = getMetricLabel(y);
+ const sizeLabel: string = getMetricLabel(size);
+
+ const refs: Refs = {};
+
+ data.forEach(datum => {
+ const name = (bubbleSeries ? datum[bubbleSeries] : datum[entity]) as string;
+ const bubbleSeriesValue = bubbleSeries ? datum[bubbleSeries] : null;
+
+ series.push({
+ name,
+ data: [
+ [
+ datum[xAxisLabel],
+ datum[yAxisLabel],
+ datum[sizeLabel],
+ datum[entity],
+ bubbleSeriesValue as any,
+ ],
+ ],
+ type: 'scatter',
+ itemStyle: { color: colorFn(name), opacity },
+ });
+ legends.push(name);
+ });
+
+ normalizeSymbolSize(series, maxBubbleSize);
+
+ const xAxisFormatter = getNumberFormatter(xAxisFormat);
+ const yAxisFormatter = getNumberFormatter(yAxisFormat);
+ const tooltipSizeFormatter = getNumberFormatter(tooltipSizeFormat);
+
+ const [min, max] = yAxisBounds.map(parseYAxisBound);
+
+ const echartOptions: EChartsCoreOption = {
+ series,
+ xAxis: {
+ axisLabel: { formatter: xAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: xAxisLabelRotation,
+ scale: true,
+ name: bubbleXAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: xAxisTitleMargin || 30,
+ type: logXAxis ? AxisType.log : AxisType.value,
+ },
+ yAxis: {
+ axisLabel: { formatter: yAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: yAxisLabelRotation,
+ scale: truncateYAxis,
+ name: bubbleYAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: yAxisTitleMargin || 50,
+ min,
+ max,
+ type: logYAxis ? AxisType.log : AxisType.value,
+ },
+ legend: {
+ ...getLegendProps(LegendType.Scroll, LegendOrientation.Top, true),
+ data: legends,
+ },
Review Comment:
Let's make the legend customizable like in the other charts, both so it can be disabled/enabled and also changeing the orientation etc (there should be controls and utils that can be readily reused)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,207 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+ getMetricLabel,
+ NumberFormatter,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType, Refs } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+import { getDefaultTooltip } from '../utils/tooltip';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+ xAxisFormatter: NumberFormatter,
+ yAxisFormatter: NumberFormatter,
+ tooltipSizeFormatter: NumberFormatter,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${xAxisFormatter(params.data[0])} <br/>
+ ${yAxisLabel}: ${yAxisFormatter(params.data[1])} <br/>
+ ${sizeLabel}: ${tooltipSizeFormatter(params.data[2])}`;
+}
+
+export default function transformProps(chartProps: EchartsBubbleChartProps) {
+ const { height, width, hooks, queriesData, formData, inContextMenu } =
+ chartProps;
+
+ const { data = [] } = queriesData[0];
+ const {
+ x,
+ y,
+ size,
+ entity,
+ maxBubbleSize,
+ colorScheme,
+ series: bubbleSeries,
+ xAxisLabel: bubbleXAxisTitle,
+ yAxisLabel: bubbleYAxisTitle,
+ xAxisFormat,
+ yAxisFormat,
+ yAxisBounds,
+ logXAxis,
+ logYAxis,
+ xAxisTitleMargin,
+ yAxisTitleMargin,
+ truncateYAxis,
+ xAxisLabelRotation,
+ yAxisLabelRotation,
+ tooltipSizeFormat,
+ opacity,
+ }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData };
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+
+ const legends: string[] = [];
+ const series: ScatterSeriesOption[] = [];
+
+ const xAxisLabel: string = getMetricLabel(x);
+ const yAxisLabel: string = getMetricLabel(y);
+ const sizeLabel: string = getMetricLabel(size);
+
+ const refs: Refs = {};
+
+ data.forEach(datum => {
+ const name = (bubbleSeries ? datum[bubbleSeries] : datum[entity]) as string;
+ const bubbleSeriesValue = bubbleSeries ? datum[bubbleSeries] : null;
+
+ series.push({
+ name,
+ data: [
+ [
+ datum[xAxisLabel],
+ datum[yAxisLabel],
+ datum[sizeLabel],
+ datum[entity],
+ bubbleSeriesValue as any,
+ ],
+ ],
+ type: 'scatter',
+ itemStyle: { color: colorFn(name), opacity },
+ });
+ legends.push(name);
+ });
+
+ normalizeSymbolSize(series, maxBubbleSize);
+
+ const xAxisFormatter = getNumberFormatter(xAxisFormat);
+ const yAxisFormatter = getNumberFormatter(yAxisFormat);
+ const tooltipSizeFormatter = getNumberFormatter(tooltipSizeFormat);
+
+ const [min, max] = yAxisBounds.map(parseYAxisBound);
+
+ const echartOptions: EChartsCoreOption = {
+ series,
+ xAxis: {
+ axisLabel: { formatter: xAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: xAxisLabelRotation,
+ scale: true,
+ name: bubbleXAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: xAxisTitleMargin || 30,
+ type: logXAxis ? AxisType.log : AxisType.value,
+ },
+ yAxis: {
+ axisLabel: { formatter: yAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: yAxisLabelRotation,
+ scale: truncateYAxis,
+ name: bubbleYAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: yAxisTitleMargin || 50,
+ min,
+ max,
+ type: logYAxis ? AxisType.log : AxisType.value,
+ },
+ legend: {
+ ...getLegendProps(LegendType.Scroll, LegendOrientation.Top, true),
+ data: legends,
+ },
+ tooltip: {
+ show: !inContextMenu,
+ ...getDefaultTooltip(refs),
+ formatter: (params: any): string =>
+ formatBubbleLabel(
+ params,
+ xAxisLabel,
+ yAxisLabel,
+ sizeLabel,
+ xAxisFormatter,
+ yAxisFormatter,
+ tooltipSizeFormatter,
+ ),
+ },
+ grid: { ...defaultGrid },
+ };
+
+ const { onContextMenu, setDataMask = () => {} } = hooks;
+
+ return {
+ height,
Review Comment:
we need to pass refs here to the component:
```suggestion
refs,
height,
```
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035587522
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -191,17 +210,21 @@ export default function transformProps(chartProps: EchartsBubbleChartProps) {
tooltipSizeFormatter,
),
},
- grid: { ...defaultGrid },
+ grid: { ...defaultGrid, ...padding },
};
const { onContextMenu, setDataMask = () => {} } = hooks;
return {
+ refs,
height,
width,
echartOptions,
onContextMenu,
setDataMask,
+ // emitFilter,
Review Comment:
wanted to keep cross filter related params in comment, will be used in next PR when its added to this chart.
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "mayurnewase (via GitHub)" <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1726608409
Hey thanks for taking this up, I am ok with both methods.
Added you as a collaborator in my fork of this repo, and I see ` Maintainers are allowed to edit this pull request.` in the right panel.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034363646
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
having 3 seaparate seems bit overkill.
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1341691144
@rusackas Ephemeral environment spinning up at http://54.186.55.19:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] rusackas commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1350228832
There are a few things from the old chart that aren't supported in this new one
• Display of X bounds and Wy bounds (the extent numbers on the axes)
• Auto-rotation of axis labels
• Axis limit inputs (love 'em or hate 'em)
• Series limit (this one seems like it would be important)
• Max Bubble Size
While I think _most_ people would be fine with _most_ of the above, it might be a bit contentious to call it a drop-in replacement without complete feature parity. We've learned this lesson before and been stuck with difficult reverts. I see a few options:
1) Go for complete feature parity (which we might not want to!)
2) Post a DISCUSS thread asking for lazy consensus on the dev list, to see if others are OK with this change, and proceed accordingly
3) Add this as a new chart, mark the current one as legacy, and live with both in the product until we can make a breaking change (3.0)
I'm thinking option 2 might be prudent. I LOVE this PR and would love to see any NVD3 chart die, but I want to make sure we're not pushing any (admittedly small) breaking changes on people without at least some form of official consent.
--
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
[GitHub] [superset] rusackas commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1350158930
Is there any intent to support temporal or categorical X-axis on this?
Also, the ephemeral environment is up and running if any of the earlier reviewers wouldn't mind giving this another pass.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1030062891
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,183 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
Review Comment:
didn't use echart's sanitize function, it removes line breaks.
--
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
[GitHub] [superset] villebro commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1033502536
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
+}
+
+export default function transformProps(chartProps: EchartsBubbleChartProps) {
+ const { height, width, hooks, queriesData, formData, inContextMenu } =
+ chartProps;
+
+ const { data = [] } = queriesData[0];
+ const {
+ x,
+ y,
+ size,
+ entity,
+ maxBubbleSize,
+ colorScheme,
+ series: bubbleSeries,
+ xAxisLabel: bubbleXAxisTitle,
+ yAxisLabel: bubbleYAxisTitle,
+ xAxisFormat,
+ yAxisFormat,
+ yAxisBounds,
+ logXAxis,
+ logYAxis,
+ xAxisTitleMargin,
+ yAxisTitleMargin,
+ truncateYAxis,
+ xAxisLabelRotation,
+ yAxisLabelRotation,
+ }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData };
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+
+ const legends: string[] = [];
+ const series: ScatterSeriesOption[] = [];
+
+ const xAxisLabel: string = x.label || x;
+ const yAxisLabel: string = y.label || y;
+ const sizeLabel: string = size.label || size;
+
+ data.forEach(datum => {
+ const name = (bubbleSeries ? datum[bubbleSeries] : datum[entity]) as string;
+ const bubbleSeriesValue = bubbleSeries ? datum[bubbleSeries] : null;
+
+ series.push({
+ name,
+ data: [
+ [
+ datum[xAxisLabel],
+ datum[yAxisLabel],
+ datum[size.label],
+ datum[entity],
+ bubbleSeriesValue as any,
+ ],
+ ],
+ type: 'scatter',
+ itemStyle: { color: colorFn(name) },
+ });
+ legends.push(name);
+ });
+
+ normalizeSymbolSize(series, maxBubbleSize);
+
+ const xAxisFormatter = getNumberFormatter(xAxisFormat);
+ const yAxisFormatter = getNumberFormatter(yAxisFormat);
+
+ const [min, max] = yAxisBounds.map(parseYAxisBound);
+
+ const echartOptions: EChartsCoreOption = {
+ series,
+ xAxis: {
+ axisLabel: { formatter: xAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: xAxisLabelRotation,
+ scale: true,
+ name: bubbleXAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: xAxisTitleMargin || 30,
+ type: logXAxis ? AxisType.log : AxisType.value,
+ },
+ yAxis: {
+ axisLabel: { formatter: yAxisFormatter },
+ splitLine: {
+ lineStyle: {
+ type: 'dashed',
+ },
+ },
+ nameRotate: yAxisLabelRotation,
+ scale: truncateYAxis,
+ name: bubbleYAxisTitle,
+ nameLocation: 'middle',
+ nameTextStyle: {
+ fontWight: 'bolder',
+ },
+ nameGap: yAxisTitleMargin || 50,
+ min,
+ max,
+ type: logYAxis ? AxisType.log : AxisType.value,
+ },
+ legend: {
+ ...getLegendProps(LegendType.Scroll, LegendOrientation.Top, true),
+ data: legends,
+ },
+ tooltip: {
+ show: !inContextMenu,
+ ...defaultTooltip,
Review Comment:
`defaultTooltip` seems to have been a leftover from a recent fix (my bad; I should have removed it in #22218). Please use `getDefaultTooltip` from `utils/tooltip.ts` (this ensures proper tooltip positioning) and remove `defaultTooltip` in `defaults.ts`:
```suggestion
...getDefaultTooltip(refs),
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
+}
+
+export default function transformProps(chartProps: EchartsBubbleChartProps) {
+ const { height, width, hooks, queriesData, formData, inContextMenu } =
+ chartProps;
+
+ const { data = [] } = queriesData[0];
+ const {
+ x,
+ y,
+ size,
+ entity,
+ maxBubbleSize,
+ colorScheme,
+ series: bubbleSeries,
+ xAxisLabel: bubbleXAxisTitle,
+ yAxisLabel: bubbleYAxisTitle,
+ xAxisFormat,
+ yAxisFormat,
+ yAxisBounds,
+ logXAxis,
+ logYAxis,
+ xAxisTitleMargin,
+ yAxisTitleMargin,
+ truncateYAxis,
+ xAxisLabelRotation,
+ yAxisLabelRotation,
+ }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData };
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+
+ const legends: string[] = [];
+ const series: ScatterSeriesOption[] = [];
+
+ const xAxisLabel: string = x.label || x;
+ const yAxisLabel: string = y.label || y;
+ const sizeLabel: string = size.label || size;
+
+ data.forEach(datum => {
+ const name = (bubbleSeries ? datum[bubbleSeries] : datum[entity]) as string;
+ const bubbleSeriesValue = bubbleSeries ? datum[bubbleSeries] : null;
+
+ series.push({
+ name,
+ data: [
+ [
+ datum[xAxisLabel],
+ datum[yAxisLabel],
+ datum[size.label],
Review Comment:
missed this one:
```suggestion
datum[sizeLabel],
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
We should probably use the formatters here for x and y values. In some other plugins I believe we have separate formatters for the axes and the tooltip - I think that could be appropriate here. Also, should we perhaps have a formatter for the bubble size, too?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
+}
+
+export default function transformProps(chartProps: EchartsBubbleChartProps) {
+ const { height, width, hooks, queriesData, formData, inContextMenu } =
+ chartProps;
+
+ const { data = [] } = queriesData[0];
+ const {
+ x,
+ y,
+ size,
+ entity,
+ maxBubbleSize,
+ colorScheme,
+ series: bubbleSeries,
+ xAxisLabel: bubbleXAxisTitle,
+ yAxisLabel: bubbleYAxisTitle,
+ xAxisFormat,
+ yAxisFormat,
+ yAxisBounds,
+ logXAxis,
+ logYAxis,
+ xAxisTitleMargin,
+ yAxisTitleMargin,
+ truncateYAxis,
+ xAxisLabelRotation,
+ yAxisLabelRotation,
+ }: EchartsBubbleFormData = { ...DEFAULT_FORM_DATA, ...formData };
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+
+ const legends: string[] = [];
+ const series: ScatterSeriesOption[] = [];
+
+ const xAxisLabel: string = x.label || x;
+ const yAxisLabel: string = y.label || y;
+ const sizeLabel: string = size.label || size;
Review Comment:
This will only work for adhoc metrics, use `getMetricLabel` from `@superset-ui/core` to make it also support saved metrics:
```suggestion
const xAxisLabel: string = getMetricLabel(x);
const yAxisLabel: string = getMetricLabel(y);
const sizeLabel: string = getMetricLabel(size);
```
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "mayurnewase (via GitHub)" <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1632849114
@EugeneTorap unfortunately no, feel free to take over or close this PR.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035629813
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
should I make them optional?
it will need modification in core, which again needs 100% coverage which will increase the PR size.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035629813
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
should make them optional?
--
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
[GitHub] [superset] EugeneTorap commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "EugeneTorap (via GitHub)" <gi...@apache.org>.
EugeneTorap commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1625724474
Hi @mayurnewase! Do you have some time to finish the PR?
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #22107: feat: Adds the ECharts Bubble chart
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1331564039
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
Resolved with `Partial`.
--
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
[GitHub] [superset] villebro commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034396696
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
Looking at the example chart in the World Bank Dashboard, the size is in billions, while t x-axis and y-axis are essentially percentages:
<img width="765" alt="image" src="https://user-images.githubusercontent.com/33317356/204468139-f738e78a-c550-4129-9efb-6c4b9b0210cb.png">
I think x and y axes will usually be in the same scale, but since we already have separate formatters for them and no separate tooltip formatter, I would maybe just propose adding a size formatter, and then using the same formatters in the tooltip for now (later on if people really want to use separate formatters on the tooltip we can add that, but let's not go for 6 formatters quite yet 😬).
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035627001
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
its because `SqlaFormData` has many values which are not set default like viz_type, datasource etc.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1030231337
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,183 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
Review Comment:
`* 2` to have same size options as previous charts in the control panel to avoid db migration.
--
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
[GitHub] [superset] john-bodley commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1646310595
cc @michael-s-molina for context who has been working on other EChart migrations.
--
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
[GitHub] [superset] villebro commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1331957379
FYI @mayurnewase @zhaoyongjie , slightly related, I'm opening a PR today that will clean up deprecated limit control logic from superset-ui/core so we can remove those annoying deprecation warnings on chart data requests
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1030063615
##########
superset-frontend/.storybook/main.js:
##########
@@ -24,6 +24,7 @@ module.exports = {
builder: 'webpack5',
},
stories: [
+ '../packages/superset-ui-demo/storybook/**/Stories.@(tsx|jsx|mdx)',
Review Comment:
bycatch: should include stories for plugins when storybook server is run.
--
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
[GitHub] [superset] rusackas commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1341688767
/testenv up
--
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
Re: [PR] feat: Adds the ECharts Bubble chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1748657955
Thanks for the review @villebro!
> But as discussed a few months ago, I think we have some tooltip harmonization and re-styling work to do anyway, so maybe this can be deferred to that effort.
Yes, let's tackle this in a follow-up as part of the harmonization and re-styling work.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034795945
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
added.
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1333491588
updated series limit to row limit, and added order control.
@zhaoyongjie the `normalizeOrderBy` in core doesn't support this control panel yet and I didn't want to change anything in core in this PR so added order support in the buildQuery itself.
Either this or need to something more hacky like below
`orderby: normalizeOrderBy(
{
...baseQueryObject,
orderby: [
[...baseQueryObject.orderby, !baseQueryObject.order_desc]
]
}
).orderby`
what do you think?
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1350261538
1. Display of X bounds and Wy bounds (the extent numbers on the axes) -> here bounds are visible by default.
2. Axis limit inputs -> just like old chart this one has Y axis limit -> need to click on `Truncate Y Axis`
3. Auto rotation -> I didnt add this for consistency, as its not present in other echart charts.
4. Series Limit -> has been changed to Row Limit as per review from @zhaoyongjie above to be inline with newer updates.
5. Max bubble size -> its there in customize section.
I tested above on ephemeral deployment.
--
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
[GitHub] [superset] michael-s-molina commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1726534975
Hi @mayurnewase. Thank you for this contribution. I would like to continue the work and see if we can ship this feature. To do that I can push to your PR if you give me the GitHub permission or I can close this PR and open my own and reference you as the original author. Which method do you prefer?
--
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
Re: [PR] feat: Adds the ECharts Bubble chart [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1748658894
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034386034
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
added 1, let me know if we need 3 separate controls.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034410185
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
agree, will add 1 for size number in tooltip and reuse axes formatters for x and y values in tooltip.
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035608163
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
Could remove the `ts-ignore` in the new plugins?
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035629813
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
should I make them optional?
--
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
[GitHub] [superset] michael-s-molina commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1727519343
> Added you as a collaborator in my fork of this repo, and I see Maintainers are allowed to edit this pull request. in the right panel.
Thanks @mayurnewase. I'll rebase the PR and modify its scope a little bit to avoid introducing a breaking change. I'll move the migration script to its own PR, similar to https://github.com/apache/superset/pull/23973 and also keep the legacy plugin. The new scope will be to just add the ECharts Bubble chart which will allow us to merge this PR without the need to wait for a major version release.
--
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
[GitHub] [superset] michael-s-molina commented on pull request #22107: feat: Adds the ECharts Bubble chart
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1728200851
/testenv up
--
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
[GitHub] [superset] codecov[bot] commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1324567996
# [Codecov](https://codecov.io/gh/apache/superset/pull/22107?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22107](https://codecov.io/gh/apache/superset/pull/22107?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c4618e) into [master](https://codecov.io/gh/apache/superset/commit/2f0d5f16f3c73a15019c7a90494ec2c03a7d4db4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f0d5f1) will **decrease** coverage by `11.47%`.
> The diff coverage is `n/a`.
> :exclamation: Current head 2c4618e differs from pull request most recent head 2fbc20b. Consider uploading reports for the commit 2fbc20b to get more accurate results
```diff
@@ Coverage Diff @@
## master #22107 +/- ##
===========================================
- Coverage 66.99% 55.51% -11.48%
===========================================
Files 1832 1832
Lines 69922 69922
Branches 7571 7571
===========================================
- Hits 46841 38820 -8021
- Misses 21122 29143 +8021
Partials 1959 1959
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.49% <ø> (ø)` | |
| python | `57.41% <ø> (-23.98%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.85% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `100.00% <ø> (ø)` | |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.61% <0.00%> (-69.39%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| ... and [285 more](https://codecov.io/gh/apache/superset/pull/22107/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1030077931
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/index.ts:
##########
@@ -0,0 +1,57 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Behavior, ChartMetadata, ChartPlugin, t } from '@superset-ui/core';
+import thumbnail from './images/thumbnail.png';
+import transformProps from './transformProps';
+import buildQuery from './buildQuery';
+import controlPanel from './controlPanel';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+
+export default class EchartsBubbleChartPlugin extends ChartPlugin<
+ EchartsBubbleFormData,
+ EchartsBubbleChartProps
+> {
+ constructor() {
+ super({
+ buildQuery,
+ controlPanel,
+ loadChart: () => import('./EchartsBubble'),
+ metadata: new ChartMetadata({
+ behaviors: [Behavior.INTERACTIVE_CHART],
+ category: t('Correlation'),
+ credits: ['https://echarts.apache.org'],
+ description: t(
+ 'Visualizes a metric across three dimensions of data in a single chart (X axis, Y axis, and bubble size). Bubbles from the same group can be showcased using bubble color.',
+ ),
+ name: t('Bubble Chart V2'),
Review Comment:
will revert to original `Bubble Chart` in next commit.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1034363302
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { EChartsCoreOption, ScatterSeriesOption } from 'echarts';
+import { extent } from 'd3-array';
+import {
+ CategoricalColorNamespace,
+ getNumberFormatter,
+ AxisType,
+} from '@superset-ui/core';
+import { EchartsBubbleChartProps, EchartsBubbleFormData } from './types';
+import { DEFAULT_FORM_DATA, MINIMUM_BUBBLE_SIZE } from './constants';
+import { defaultGrid, defaultTooltip } from '../defaults';
+import { getLegendProps } from '../utils/series';
+import { LegendOrientation, LegendType } from '../types';
+import { parseYAxisBound } from '../utils/controls';
+
+function normalizeSymbolSize(
+ nodes: ScatterSeriesOption[],
+ maxBubbleValue: number,
+) {
+ const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x => x.data![0][2]);
+ const nodeSpread = bubbleMaxValue - bubbleMinValue;
+ nodes.forEach(node => {
+ // eslint-disable-next-line no-param-reassign
+ node.symbolSize =
+ (((node.data![0][2] - bubbleMinValue) / nodeSpread) *
+ (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
+ });
+}
+
+export function formatBubbleLabel(
+ params: any,
+ xAxisLabel: string,
+ yAxisLabel: string,
+ sizeLabel: string,
+) {
+ const title = params.data[4]
+ ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+ : params.data[3];
+
+ return `<p>${title}</p>
+ ${xAxisLabel}: ${params.data[0]} <br/>
+ ${yAxisLabel}: ${params.data[1]} <br/>
+ ${sizeLabel}: ${params.data[2]}`;
Review Comment:
should we have single formatter for all 3 values?
--
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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035591158
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/buildQuery.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { buildQueryContext, QueryFormData } from '@superset-ui/core';
+
+export default function buildQuery(formData: QueryFormData) {
+ const columns = [formData.entity];
+ if (formData.series) columns.push(formData.series);
+
+ return buildQueryContext(formData, baseQueryObject => [
+ {
+ ...baseQueryObject,
+ columns,
Review Comment:
to use array spread
```typescript
columns: [...ensureIsArray(formData.entity), ...ensureIsArray(formData.series)],
```
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1330719634
##########
superset-frontend/.storybook/main.js:
##########
@@ -24,6 +24,7 @@ module.exports = {
builder: 'webpack5',
},
stories: [
+ '../packages/superset-ui-demo/storybook/**/Stories.@(tsx|jsx|mdx)',
Review Comment:
Actually they are separate storybooks currently. Before merging them, we need to think about how to organize the components.
--
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
Re: [PR] feat: Adds the ECharts Bubble chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #22107:
URL: https://github.com/apache/superset/pull/22107
--
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
[GitHub] [superset] michael-s-molina commented on pull request #22107: feat: Adds the ECharts Bubble chart
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1728096809
I deprecated the legacy chart and added thumbnails for the new version.
<img width="1063" alt="Screenshot 2023-09-20 at 13 41 49" src="https://github.com/apache/superset/assets/70410625/07a82cce-c358-483f-abae-5e76a6decc50">
--
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
[GitHub] [superset] rusackas commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1334410152
Just seeing this and catching up on the thread - this is great, thanks!
I haven't looked at the code yet, but for myself or any other reviewer, I'm hopeful that this (or a followup) includes the latest best practices:
• Generic X Axis
• Standardized controls (normalizing/denormalizing for fast-viz switching)
• Drill to Detail support
Basically, trying to keep all the ECharts plugins current with all the first-class bells and whistles. Happy to provide support/references for any of the above if needed.
Again, thanks, this is exciting!
--
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
[GitHub] [superset] zhaoyongjie commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1331771759
the `series limit` will send a subquery instead a simple query, the time-series chart should be referenced.
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1331798335
> @mayurnewase the `series limit` will send a subquery instead a simple query, the time-series chart should be referenced.
I see, let me check.
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1334650225
> Just seeing this and catching up on the thread - this is great, thanks!
>
> I haven't looked at the code yet, but for myself or any other reviewer, I'm hopeful that this (or a followup) includes the latest best practices: • Generic X Axis • Standardized controls (normalizing/denormalizing for fast-viz switching) • Drill to Detail support
>
> Basically, trying to keep all the ECharts plugins current with all the first-class bells and whistles. Happy to provide support/references for any of the above if needed.
>
> Again, thanks, this is exciting!
@rusackas
1. x and y axes are both metrics
2. controls are standard but for fast switching don't think there is any other chart with x and y and 1 more metric for bubble size, so not sure how I can test the switch.
3. I will add cross filter and drill to detail as soon as this PR gets merged, because it may need other modifications and new tests.
possible to get 1 more review?
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1330700879
> First pass comments, looks great! One thing that I noticed is that the NVD3 plugin assumes truncation on the y-axis if the bounds aren't set. While I don't necessarily like the idea of defaulting to truncation (actually, I strongly oppose it), I was just wondering if we should do a migration that enables truncation for existing charts if the bounds are undefined?
agree, added migration.
--
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
[GitHub] [superset] mayurnewase commented on a diff in pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on code in PR #22107:
URL: https://github.com/apache/superset/pull/22107#discussion_r1035632408
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts:
##########
@@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DEFAULT_LEGEND_FORM_DATA } from '../constants';
+import { EchartsBubbleFormData } from './types';
+
+// @ts-ignore
Review Comment:
it will need modification in core, which again needs 100% coverage which will increase the PR size.
--
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
[GitHub] [superset] mayurnewase commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by GitBox <gi...@apache.org>.
mayurnewase commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1331760108
> I just saw the original bubble chart(NVD3 version), and the `limit control` have an inconsistent setting. IMO, we should fix/improve that in the **new version** of Bubble Chart.
>
> 1. The `Series Limit` refers to a limit by series rather than a `row limit`.
> 2. No chance to set a `order by` clause in the SQL
>
> 
here series limit made sense as its inline with echart's options.(every bubble is separate series)
and description of `dimension` in control panel had series in it to group them by color.
--
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
[GitHub] [superset] rusackas commented on pull request #22107: feat: migrate bubble chart to echarts
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1409049687
@mayurnewase this appears to need a rebase. It might also need some commits added that change the old bubble chart name to include "legacy." @villebro has done some of this type of renaming and can probably warn us of any intricacies here.
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #22107: feat: Adds the ECharts Bubble chart
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22107:
URL: https://github.com/apache/superset/pull/22107#issuecomment-1728203476
@michael-s-molina Ephemeral environment spinning up at http://34.217.102.24:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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