Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mixed time-series; time-series bar chart v2] D3 format of saved metric is ignored; bar values ignore D3 format of the chart #18234

Closed
3 tasks done
rumbin opened this issue Jan 31, 2022 · 3 comments
Labels
#bug Bug report good first issue Good first issues for new contributors preset:cares Preset cares about this issue validation:validated A committer has validated / submitted the issue or it was reported by multiple users

Comments

@rumbin
Copy link
Contributor

rumbin commented Jan 31, 2022

I can replicate this on both, mixed time-series and time-series bar chart v2.

The D3 format of a saved metric is not respected in the

  • y-axis labels
  • tooltips
  • Show Values

How to reproduce the bug

  1. Have a saved metric defined for a dataset
  2. Have a custom D3 format defined for this metric
  3. Create a diagram of type mixed time-series or ** time-series bar chart v2**
  4. Activate the Show Values option

Expected results

The y-axis labels, the tooltips and the bar values are formatted according to the defined D3 format of the metric.

Actual results

The d3 format of the metric is entirely ignored.

When overriding the y-axis format via the chart's Customize settings, the y-labels and tooltips are respecting this override.
The bar values need some differentiation here, though:

  • time-series bar chart v2: respects the chart's y-axis D3 format
  • mixed time-series: does not respect the chart's y-axis D3 format

Screenshots

  • time-series bar chart v2

bar_chart_d3_format

  • mixed time-series: (also notice how the bar values are not customizable here)

mixed_time-series_d3_format

Environment

  • browser type and version: Chrome 96.0.4664.93
  • superset version: 1.4.0
  • all other versions: as defined in Superset's Docker image
  • Feature flags:
# stable:
      "THUMBNAILS": True,
      "SQLLAB_BACKEND_PERSISTENCE": True,
      "ENABLE_TEMPLATE_PROCESSING": True,
      "DASHBOARD_CROSS_FILTERS": True,

      # experimental:
      "ALERT_REPORTS": True,
      "ALERTS_ATTACH_REPORTS": True,
      "DASHBOARD_NATIVE_FILTERS": True,

      # unclassified:
      "ENABLE_EXPLORE_DRAG_AND_DROP": True,
      "ENABLE_DND_WITH_CLICK_UX": True,

      # development:
      "DASHBOARD_NATIVE_FILTERS_SET": True

Checklist

Make sure to follow these steps before submitting your issue - thank you!

  • I have checked the superset logs for python stacktraces and included it here as text if there are any.
  • I have reproduced the issue with at least the latest released version of superset.
  • I have checked the issue tracker for the same issue and I haven't found one similar.
@rumbin rumbin added the #bug Bug report label Jan 31, 2022
@geido geido added good first issue Good first issues for new contributors preset:cares Preset cares about this issue validation:validated A committer has validated / submitted the issue or it was reported by multiple users labels Feb 1, 2022
@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 17, 2022
@openaphid
Copy link

I managed to come up with a quick fix for this issue, which works for my own charts:

diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
index 5b3b5523e..41e7fa356 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
@@ -95,7 +95,7 @@ export default function transformProps(
     theme,
     annotationData = {},
   } = chartProps;
-  const { verboseMap = {} } = datasource;
+  const { verboseMap = {}, columnFormats } = datasource;
   const [queryData] = queriesData;
   const { data = [] } = queryData as TimeseriesChartDataResponseResult;
   const dataTypes = getColtypesMapping(queryData);
@@ -140,6 +140,7 @@ export default function transformProps(
     sliceId,
     timeGrainSqla,
     orientation,
+    metrics,
   }: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
 
   const colorScale = CategoricalColorNamespace.getScale(colorScheme as string);
@@ -173,9 +174,34 @@ export default function transformProps(
   const xAxisDataType = dataTypes?.[xAxisCol];
   const xAxisType = getAxisType(xAxisDataType);
   const series: SeriesOption[] = [];
-  const formatter = getNumberFormatter(
-    contributionMode || isAreaExpand ? ',.0%' : yAxisFormat,
-  );
+
+  // fix https://github.com/apache/superset/issues/18234
+  const funcFindFormatter = (seriesName) => {
+    let bestFormat = yAxisFormat
+    if (bestFormat === 'SMART_NUMBER') { // if it's not specified in chart options
+      if (metrics.length === 1) { // only one metric, seriesName is not prefixed with metric name
+        const format = columnFormats[metrics[0] || ""]
+        if (format)
+          bestFormat = format
+      } else if (seriesName) { // the chart has multiple metrics, the format can be extracted from prefix
+        for (let m of metrics) {
+          if (seriesName.startsWith(m + ',')) {
+            const format = columnFormats[m]
+            if (format) {
+              bestFormat = format
+            }
+            break
+          }
+        }
+      }
+    } 
+
+    const formatter = getNumberFormatter(
+      contributionMode || isAreaExpand ? ',.0%' : bestFormat,
+    );  
+
+    return formatter
+  }
 
   rawSeries.forEach(entry => {
     const lineStyle = isDerivedSeries(entry, chartProps.rawFormData)
@@ -190,7 +216,7 @@ export default function transformProps(
       areaOpacity: opacity,
       seriesType,
       stack,
-      formatter,
+      formatter: funcFindFormatter(entry.id),
       showValue,
       onlyTotal,
       totalStackedValues,
@@ -337,7 +363,7 @@ export default function transformProps(
     max,
     minorTick: { show: true },
     minorSplitLine: { show: minorSplitLine },
-    axisLabel: { formatter },
+    axisLabel: { formatter: funcFindFormatter('') }, 
     scale: truncateYAxis,
     name: yAxisTitle,
     nameGap: convertInteger(yAxisTitleMargin),
@@ -380,7 +406,7 @@ export default function transformProps(
           const content = formatForecastTooltipSeries({
             ...value,
             seriesName: key,
-            formatter,
+            formatter: funcFindFormatter(key),
           });
           if (currentSeries.name === key) {
             rows.push(`<span style="font-weight: 700">${content}</span>`);

@stale stale bot removed the inactive Inactive for >= 30 days label Jun 30, 2022
@rusackas
Copy link
Member

rusackas commented Feb 7, 2024

Closing this as stale since it's been silent for so long, and we're trying to steer toward a more actionable Issues backlog. If people are still encountering this in current versions (currently 3.x) please open a new Issue or a PR to address the problem.

@rusackas rusackas closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#bug Bug report good first issue Good first issues for new contributors preset:cares Preset cares about this issue validation:validated A committer has validated / submitted the issue or it was reported by multiple users
Projects
None yet
Development

No branches or pull requests

4 participants