fix(plugin-charts): data labels print in their value axis's units and the tallest mark keeps its label on the chart

.changeset/27372-chart-data-label-units-headroom.md

@@ -0,0 +1,11 @@
+---
+'@object-ui/plugin-charts': patch
+---
+
+fix(plugin-charts): a chart's data labels print in their value axis's units, and the tallest mark keeps its label on the chart surface
+
+Clause-②: no
+Metadata-protocol: none
+
+- **Units.** With `showDataLabels` on, each value label now prints with the tick formatter of the value axis its series is drawn against. An axis with no authored `format` labels its marks in the same compact units as its ticks (`174K` beside a `400K` tick, where the label used to print `174000`); an authored `yAxis[].format` still wins, per axis on a dual-axis or combo chart. Dashboard charts, whose `chartConfig` carries no axis format, are the common case.
+- **Headroom.** A vertical bar, line, area or combo chart with data labels on takes a top margin that fits one label above the plot, so a mark on the top edge of the domain (the tallest bar under a nice top tick) no longer draws its label outside the chart. Charts without data labels, and horizontal bars, keep recharts' default margin.

ui/packages/plugin-charts/src/AdvancedChartImpl.dataLabels-27372.test.tsx

@@ -0,0 +1,160 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * objectstack#27372 — a chart's data labels (`showDataLabels`) read in the
+ * units of their value axis, and the tallest mark keeps its label on the
+ * chart surface.
+ *
+ * Two defects, one label:
+ *   - The value axis falls back to the compact formatter (`400K`) when no
+ *     `yAxis.format` is authored; the data label had no fallback and printed
+ *     the raw number (`174000` beside a `400K` tick).
+ *   - The label sits above its mark. With recharts' default 5px top margin, a
+ *     mark on the plot's top edge (the domain's nice max) had its label drawn
+ *     wholly above the chart surface.
+ *
+ * happy-dom paints no text, but given a fixed box recharts computes every mark
+ * and label position arithmetically (the same property
+ * `AdvancedChartImpl.pointAxisEdgeLabels-13390.test.tsx` relies on), so these
+ * pins read the label's text and its `y` from the DOM.
+ */
+import React from 'react';
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { render, cleanup } from '@testing-library/react';
+
+const WIDTH = 520;
+const HEIGHT = 300;
+
+vi.mock('recharts', async () => {
+  const actual = await vi.importActual<typeof import('recharts')>('recharts');
+  return {
+    ...actual,
+    ResponsiveContainer: ({ children }: { children: React.ReactElement<{ width: number; height: number }> }) =>
+      React.cloneElement(children, { width: WIDTH, height: HEIGHT }),
+  };
+});
+
+import AdvancedChartImpl from './AdvancedChartImpl';
+
+afterEach(cleanup);
+
+/** The card's shape: the tallest value lands on a nice top tick (400K). */
+const PIPELINE = [
+  { stage: 'Prospecting', amount: 174000, won: 12 },
+  { stage: 'Qualification', amount: 400000, won: 30 },
+  { stage: 'Proposal', amount: 251000, won: 18 },
+  { stage: 'Negotiation', amount: 92500, won: 7 },
+];
+
+/** The font size the chart prints data labels in. */
+const LABEL_FONT_PX = 11;
+
+type Family = NonNullable<React.ComponentProps<typeof AdvancedChartImpl>['chartType']>;
+
+function draw(chartType: Family, extra: Record<string, unknown> = {}) {
+  const { container } = render(
+    <AdvancedChartImpl
+      chartType={chartType}
+      data={PIPELINE}
+      xAxisKey="stage"
+      series={[{ dataKey: 'amount', label: 'Amount' }]}
+      showLegend={false}
+      showDataLabels
+      isAnimationActive={false}
+      {...extra}
+    />,
+  );
+  return container;
+}
+
+/** Each data label: its text and the `y` its baseline is drawn at. */
+function labelsOf(container: HTMLElement) {
+  const texts = [...container.querySelectorAll('.recharts-label-list text')];
+  expect(texts.length, 'the chart drew no data labels, so this measures nothing').toBeGreaterThan(0);
+  return texts.map((t) => ({ text: (t.textContent ?? '').trim(), y: Number(t.getAttribute('y')) }));
+}
+
+/** The y-axis tick texts, so a label can be read against its own axis. */
+function yTicksOf(container: HTMLElement, axis = '.recharts-yAxis-tick-labels') {
+  return [...container.querySelectorAll(`${axis} .recharts-cartesian-axis-tick-value`)].map((t) =>
+    (t.textContent ?? '').trim(),
+  );
+}
+
+describe('objectstack#27372 — a data label reads in its value axis’s units', () => {
+  for (const chartType of ['bar', 'line', 'area'] as const) {
+    it(`prints the compact default beside a compact ${chartType} axis when no format is authored`, () => {
+      const container = draw(chartType);
+      // The axis is the reference: it must itself be compact, or the label
+      // matching it would prove nothing about units.
+      expect(yTicksOf(container)).toContain('400K');
+      expect(labelsOf(container).map((l) => l.text)).toEqual(['174K', '400K', '251K', '92.5K']);
+    });
+  }
+
+  it('lets an authored axis format win over the compact default', () => {
+    const container = draw('bar', { yAxes: [{ field: 'amount', format: '$0,0' }] });
+    expect(labelsOf(container).map((l) => l.text)).toEqual(['$174,000', '$400,000', '$251,000', '$92,500']);
+  });
+
+  it('prints each series in the units of the axis it is bound to on a dual-axis chart', () => {
+    const container = draw('bar', {
+      series: [
+        { dataKey: 'amount', label: 'Amount' },
+        { dataKey: 'won', label: 'Won', yAxis: 'right' },
+      ],
+      yAxes: [{ field: 'amount' }, { field: 'won', position: 'right', format: '0.0' }],
+    });
+    expect(labelsOf(container).map((l) => l.text)).toEqual([
+      '174K', '400K', '251K', '92.5K',
+      '12.0', '30.0', '18.0', '7.0',
+    ]);
+  });
+
+  it('prints a combo series in the units of the axis it is drawn against', () => {
+    const container = draw('combo', {
+      series: [
+        { dataKey: 'amount', label: 'Amount', chartType: 'bar' },
+        { dataKey: 'won', label: 'Won', chartType: 'line', yAxis: 'right' },
+      ],
+      yAxes: [{ field: 'amount' }, { field: 'won', position: 'right', format: '0.0' }],
+    });
+    expect(labelsOf(container).map((l) => l.text)).toEqual([
+      '174K', '400K', '251K', '92.5K',
+      '12.0', '30.0', '18.0', '7.0',
+    ]);
+  });
+});
+
+describe('objectstack#27372 — the tallest mark keeps its data label on the chart surface', () => {
+  for (const chartType of ['bar', 'line', 'area', 'combo'] as const) {
+    it(`draws the top label of a ${chartType} chart inside the surface`, () => {
+      const container = draw(chartType);
+      const labels = labelsOf(container);
+      const top = labels.reduce((a, b) => (b.y < a.y ? b : a));
+      expect(top.text).toBe('400K');
+      // The label's baseline is at `y`; its glyphs rise at most one font size
+      // above it. Below that, the label is (partly) off the top of the svg.
+      expect(top.y - LABEL_FONT_PX, `"${top.text}" is drawn above the chart surface`).toBeGreaterThanOrEqual(0);
+    });
+  }
+
+  it('keeps recharts’ default top margin when data labels are off', () => {
+    const { container } = render(
+      <AdvancedChartImpl
+        chartType="bar"
+        data={PIPELINE}
+        xAxisKey="stage"
+        series={[{ dataKey: 'amount', label: 'Amount' }]}
+        showLegend={false}
+        isAnimationActive={false}
+      />,
+    );
+    const plotTop = Number(container.querySelector('defs clipPath rect')?.getAttribute('y'));
+    expect(plotTop).toBe(5);
+  });
+});

ui/packages/plugin-charts/src/AdvancedChartImpl.tsx

@@ -482,6 +482,36 @@ function estimateLabelWidthPx(label: string): number {
 /** recharts' default chart margin, left plus right (5px each side). */
 const CHART_MARGIN_X_PX = 10;
 
+/** The font size `showDataLabels` prints each point's value in, in px. */
+const DATA_LABEL_FONT_PX = 11;
+/** recharts' `LabelList` default `offset`: the gap between a mark and its label. */
+const DATA_LABEL_OFFSET_PX = 5;
+
+/**
+ * The chart margin a vertical cartesian chart (bar, line, area, combo) takes
+ * while `showDataLabels` is on (objectstack#27372).
+ *
+ * A data label sits ABOVE its mark: its baseline is `DATA_LABEL_OFFSET_PX`
+ * over the mark's top. recharts' default margin leaves 5px between the plot's
+ * top edge and the end of the chart surface, and the value axis's nice domain
+ * routinely puts the tallest mark exactly on that edge (a 400,000 bar under a
+ * 400K top tick). Its label was then drawn wholly outside the surface: the
+ * tallest bar, the one a reader looks at first, was the one with no value.
+ *
+ * The top margin is that 5px plus the label's own height (offset and font),
+ * so a mark ON the top edge still has its label inside the surface. A margin
+ * rather than a padded domain: an author's `yAxis.max` pins the domain and
+ * must keep doing so, and the margin holds whatever the domain is. The other
+ * three sides stay recharts' default 5px, which `CHART_MARGIN_X_PX` counts on.
+ * Horizontal bars are left alone: their value runs across the plot.
+ */
+const DATA_LABEL_CHART_MARGIN = {
+  top: 5 + DATA_LABEL_OFFSET_PX + DATA_LABEL_FONT_PX,
+  right: 5,
+  bottom: 5,
+  left: 5,
+} as const;
+
 /**
  * The inset, in px, a line or area chart's categorical x axis keeps at BOTH
  * ends of the plot (objectui#13390).
@@ -2128,10 +2158,6 @@ function AdvancedChartImplInner({
     yAxisPositionNotes(yPlacement, categoriesRunDown),
     yAxisUndrawnNotes(yPlacement, yAxes, categoriesRunDown),
   );
-  /** The entry a series' marks are measured against — for its value labels. */
-  const axisOfSeries = (s: { yAxis?: string }): NormalizedAxis | undefined =>
-    hasDualAxis ? (s.yAxis === 'right' ? rightY : leftY) : primaryY;
-
   const xTickFormatter = React.useMemo(
     () => formatterFor(xAxisSpec?.format, displayLocale) ?? formatTick,
     [xAxisSpec?.format, displayLocale, formatTick],
@@ -2196,6 +2222,12 @@ function AdvancedChartImplInner({
     () => formatterFor(rightY?.format, displayLocale) ?? formatYTick,
     [rightY?.format, displayLocale, formatYTick],
   );
+  /**
+   * The tick formatter of the value axis a series' marks are measured
+   * against — the one its value labels print with (objectstack#27372).
+   */
+  const valueTickFormatterOf = (s: { yAxis?: string }): typeof yTickFormatter =>
+    hasDualAxis ? (s.yAxis === 'right' ? rightYTickFormatter : leftYTickFormatter) : yTickFormatter;
 
   /**
    * Every number plotted in one slot of a dual axis (or on the only axis,
@@ -2317,10 +2349,16 @@ function AdvancedChartImplInner({
     ? <Brush dataKey={xAxisKey} height={24} travellerWidth={8} stroke="hsl(var(--muted-foreground))" />
     : null;
 
-  /** `showDataLabels` prints each point's value on the mark itself. */
-  const dataLabel = (formatter?: (v: any) => string) =>
+  /**
+   * `showDataLabels` prints each point's value on the mark itself, in the
+   * units of the value axis the mark is measured against (objectstack#27372):
+   * the caller passes that axis's own tick formatter, so an authored
+   * `yAxis.format` wins and an axis without one gives its label the compact
+   * default (`174K` beside a `400K` tick, never a raw `174000`).
+   */
+  const dataLabel = (formatter: (v: any) => string) =>
     showDataLabels
-      ? <LabelList position="top" className="fill-foreground" fontSize={11} {...(formatter ? { formatter } : {})} />
+      ? <LabelList position="top" className="fill-foreground" fontSize={DATA_LABEL_FONT_PX} offset={DATA_LABEL_OFFSET_PX} formatter={formatter} />
       : null;
 
   // Shared X-axis props for time/categorical axes, in two branches.
@@ -3034,7 +3072,7 @@ function AdvancedChartImplInner({
             happens at chart level because that is the only place the CATEGORY
             is known — a line/area item handler is handed the curve's props and
             no datum. */}
-        <ComposedChart ref={setChartSurface} data={data} {...comboClickProps}>
+        <ComposedChart ref={setChartSurface} data={data} {...(showDataLabels ? { margin: DATA_LABEL_CHART_MARGIN } : {})} {...comboClickProps}>
           <CartesianGrid {...gridProps} {...valueAxisIdFor(valueAxesHaveIds, categoriesRunDown)} />
           <XAxis dataKey={xAxisKey} {...xAxisCommonProps} />
           {/* A combo always draws both value axes (an authored combo binds an
@@ -3072,7 +3110,7 @@ function AdvancedChartImplInner({
                 : (seriesType === 'bar' ? 'left' : 'right');
             const pres = seriesStyle(s, seriesType as any);
             const stackProps = s.stack ? { stackId: String(s.stack) } : {};
-            const valueFormatter = formatterFor((yAxisId === 'right' ? rightY : leftY)?.format, displayLocale);
+            const valueFormatter = yAxisId === 'right' ? rightYTickFormatter : leftYTickFormatter;
 
             if (seriesType === 'line') {
               return (
@@ -3122,7 +3160,7 @@ function AdvancedChartImplInner({
     <ChartFrame title={title} subtitle={subtitle}>
     <ChartFootnote note={joinNotes(xPositionNote, yPositionNote)}>
     <ChartContainer config={config} className={className} {...containerProps}>
-      <ChartComponent ref={setChartSurface} data={data} layout={isHorizontal ? 'vertical' : 'horizontal'} {...cartesianClickProps}>
+      <ChartComponent ref={setChartSurface} data={data} layout={isHorizontal ? 'vertical' : 'horizontal'} {...(showDataLabels && !isHorizontal ? { margin: DATA_LABEL_CHART_MARGIN } : {})} {...cartesianClickProps}>
         <defs>
           {gradColors.map((c) => (
             <React.Fragment key={gslug(c)}>
@@ -3263,7 +3301,7 @@ function AdvancedChartImplInner({
           // Recharts keys stacking off `stackId`, so the author's group name
           // passes through unchanged.
           const stackProps = s.stack ? { stackId: String(s.stack) } : {};
-          const valueFormatter = formatterFor(axisOfSeries(s)?.format, displayLocale);
+          const valueFormatter = valueTickFormatterOf(s);
 
           if (chartType === 'bar' || chartType === 'horizontal-bar') {
             // One series, one colour: a bar's colour names its SERIES, so a