Merge pull request #2007 from malloydata/renderer-knowledge-transfer

fix: chart bugs
malloydata · Nov 13, 2024 · 2dcb325 · 2dcb325
2 parents 3df5739 + f1f73e8
commit 2dcb325
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 15 deletions.
diff --git a/packages/malloy-render/src/component/apply-renderer.tsx b/packages/malloy-render/src/component/apply-renderer.tsx
@@ -46,12 +46,13 @@ export function shouldRenderAs(f: Field | Explore, tagOverride?: Tag) {
   if (hasAny(tag, 'list', 'list_detail')) return 'list';
   if (hasAny(tag, 'bar_chart', 'line_chart')) return 'chart';
   if (tag.has('dashboard')) return 'dashboard';
-  if (hasAny(tag, 'scatter_chart', 'shape_map', 'segment_map'))
-    return 'legacy_chart';
+  if (hasAny(tag, 'scatter_chart')) return 'scatter_chart';
+  if (hasAny(tag, 'shape_map')) return 'shape_map';
+  if (hasAny(tag, 'segment_map')) return 'segment_map';
   else return 'table';
 }
 
-const NULL_SYMBOL = '∅';
+export const NULL_SYMBOL = '∅';
 
 export function applyRenderer(props: RendererProps) {
   const {field, dataColumn, resultMetadata, tag, customProps = {}} = props;
@@ -117,7 +118,9 @@ export function applyRenderer(props: RendererProps) {
           );
         break;
       }
-      case 'legacy_chart': {
+      case 'scatter_chart':
+      case 'shape_map':
+      case 'segment_map': {
         if (dataColumn.isArray())
           renderValue = <LegacyChart type={renderAs} data={dataColumn} />;
         else

diff --git a/packages/malloy-render/src/component/bar-chart/generate-bar_chart-vega-spec.ts b/packages/malloy-render/src/component/bar-chart/generate-bar_chart-vega-spec.ts
@@ -35,6 +35,7 @@ import {renderNumericField} from '../render-numeric-field';
 import {createMeasureAxis} from '../vega/measure-axis';
 import {getCustomTooltipEntries} from './get-custom-tooltips-entries';
 import {getMarkName} from '../vega/vega-utils';
+import {NULL_SYMBOL} from '../apply-renderer';
 
 type BarDataRecord = {
   x: string | number;
@@ -426,7 +427,7 @@ export function generateBarChartVegaSpec(
     },
     {
       name: 'brushMeasureIn',
-      update: 'getMalloyBrush(brushIn, yRefsList, \'measure\') || "empty"',
+      update: "getMalloyBrush(brushIn, yRefsList, 'measure') || null",
     },
     {
       name: 'brushMeasureRangeIn',
@@ -780,6 +781,7 @@ export function generateBarChartVegaSpec(
     }[] = [];
     data.forEach(row => {
       // Filter out missing date/time values
+      // TODO: figure out how we can show null values in continuous axes
       if (
         xIsDateorTime &&
         (row[xFieldPath] === null || typeof row[xFieldPath] === 'undefined')
@@ -789,7 +791,7 @@ export function generateBarChartVegaSpec(
       // Map data fields to chart properties
       mappedData.push({
         __source: row,
-        x: getXValue(row),
+        x: getXValue(row) ?? NULL_SYMBOL,
         y: row[yFieldPath],
         series: seriesFieldPath ? row[seriesFieldPath] : yFieldPath,
       });

diff --git a/packages/malloy-render/src/component/chart-layout-settings.ts b/packages/malloy-render/src/component/chart-layout-settings.ts
@@ -213,7 +213,8 @@ export function getChartLayoutSettings(
     const xSpacePerLabel = chartWidth / recordsToFit;
     if (xSpacePerLabel > xAxisHeight || xSpacePerLabel > maxStringSize) {
       labelAngle = 0;
-      labelLimit = xSpacePerLabel;
+      // Remove label limit; our vega specs should use labelOverlap setting to hide overlapping labels
+      labelLimit = 0;
       labelAlign = undefined;
       labelBaseline = 'top';
       xTitleSize = 22;

diff --git a/packages/malloy-render/src/component/chart/default-chart-tooltip.tsx b/packages/malloy-render/src/component/chart/default-chart-tooltip.tsx
@@ -7,6 +7,11 @@
 
 import {For, Match, Show, Switch, createEffect, createSignal} from 'solid-js';
 import {ChartTooltipEntry} from '../types';
+// TODO: This is a hacky way to get table CSS applied at global level so it affects tables in tooltips
+// Need a better strategy here for how we render things inside of tooltips. Is it possible to keep
+// tooltips and modals inside of the <malloy-render> component? Or can we render <malloy-render> with
+// subsets of the Result set?
+import '../table/table.css';
 
 export function DefaultChartTooltip(props: {data: ChartTooltipEntry}) {
   const [render, setRender] = createSignal(false);

diff --git a/packages/malloy-render/src/component/line-chart/generate-line_chart-vega-spec.ts b/packages/malloy-render/src/component/line-chart/generate-line_chart-vega-spec.ts
@@ -34,6 +34,7 @@ import {
 import {renderNumericField} from '../render-numeric-field';
 import {getMarkName} from '../vega/vega-utils';
 import {getCustomTooltipEntries} from '../bar-chart/get-custom-tooltips-entries';
+import {NULL_SYMBOL} from '../apply-renderer';
 
 type LineDataRecord = {
   x: string | number;
@@ -437,7 +438,7 @@ export function generateLineChartVegaSpec(
     },
     {
       name: 'brushMeasureIn',
-      update: 'getMalloyBrush(brushIn, yRefsList, \'measure\') || "empty"',
+      update: "getMalloyBrush(brushIn, yRefsList, 'measure') || null",
     },
     {
       name: 'brushMeasureRangeIn',
@@ -750,7 +751,7 @@ export function generateLineChartVegaSpec(
       // Map data fields to chart properties
       mappedData.push({
         __source: row,
-        x: getXValue(row),
+        x: getXValue(row) ?? NULL_SYMBOL,
         y: row[yFieldPath],
         series: seriesFieldPath ? row[seriesFieldPath] : yFieldPath,
       });

diff --git a/packages/malloy-render/src/component/render-result-metadata.ts b/packages/malloy-render/src/component/render-result-metadata.ts
@@ -44,7 +44,7 @@ import {
   VegaChartProps,
   VegaConfigHandler,
 } from './types';
-import {shouldRenderAs} from './apply-renderer';
+import {NULL_SYMBOL, shouldRenderAs} from './apply-renderer';
 import {mergeVegaConfigs} from './vega/merge-vega-configs';
 import {baseVegaConfig} from './vega/base-vega-config';
 import {renderTimeString} from './render-time';
@@ -165,8 +165,10 @@ const populateFieldMeta = (data: DataArray, metadata: RenderResultMetadata) => {
       const fieldSet = maxUniqueFieldValueSets.get(getFieldKey(f))!;
 
       const value = f.isAtomicField() ? row.cell(f).value : undefined;
-
-      if (valueIsNumber(f, value)) {
+      if (value === null || typeof value === 'undefined') {
+        fieldMeta.values.add(NULL_SYMBOL);
+        fieldSet.add(NULL_SYMBOL);
+      } else if (valueIsNumber(f, value)) {
         const n = value;
         fieldMeta.min = Math.min(fieldMeta.min ?? n, n);
         fieldMeta.max = Math.max(fieldMeta.max ?? n, n);

diff --git a/packages/malloy-render/src/component/vega/measure-axis.ts b/packages/malloy-render/src/component/vega/measure-axis.ts
@@ -52,7 +52,7 @@ export function createMeasureAxis({
             ...(showBrushes
               ? [
                   {
-                    test: 'brushMeasureIn !== "empty" ? (datum.index !== 0 && datum.index !== 1) : false',
+                    test: 'brushMeasureIn !== null ? (datum.index !== 0 && datum.index !== 1) : false',
                     value: 0,
                   },
                   {
@@ -152,14 +152,14 @@ type AxisReferenceLineOptions = {
 
 function createAxisReferenceLines(options: AxisReferenceLineOptions) {
   const opacityRefLineSignal = {
-    signal: 'brushMeasureIn === "empty" || yIsBrushing ? 0 : 1',
+    signal: 'brushMeasureIn === null || yIsBrushing ? 0 : 1',
   };
 
   const startingXPosition =
     -options.axisSettings.width + options.axisSettings.yTitleSize;
 
   const yPositionSignalWithOffset = (offset = 0) =>
-    `brushMeasureIn !== "empty" ? (scale("yscale",brushMeasureIn) + ${offset}) : 0`;
+    `brushMeasureIn !== null ? (scale("yscale",brushMeasureIn) + ${offset}) : 0`;
 
   const referenceLines: GroupMark = {
     name: 'y_reference_line_group',

diff --git a/packages/malloy-render/src/stories/bar_chart.stories.malloy b/packages/malloy-render/src/stories/bar_chart.stories.malloy
@@ -458,6 +458,17 @@ source: products is duckdb.table("static/data/products.parquet") extend {
 
   #(story)
   view: interactions_sandbox is interactions;
+
+  #(story)
+  # bar_chart.stack
+  view: null_values is {
+    group_by:
+      x is pick null when id = 8 else id
+      id
+    aggregate: `Sales $`
+    limit: 10
+    order_by: id
+  }
 }
 
 run: products -> { group_by: distribution_center_id}