Make timeseries tick spacing depend on the current format (#11268)

frontend/src/metabase/visualizations/lib/apply_axis.js

@@ -122,12 +122,24 @@ export function applyChartTimeseriesXAxis(
     // special handling for weeks
     // TODO: are there any other cases where we should do this?
     let tickFormatUnit = dimensionColumn.unit;
+    const tickFormat = timestamp => {
+      const { column, ...columnSettings } = chart.settings.column(
+        dimensionColumn,
+      );
+      return formatValue(timestamp, {
+        ...columnSettings,
+        column: { ...column, unit: tickFormatUnit },
+        type: "axis",
+        compact: chart.settings["graph.x_axis.axis_enabled"] === "compact",
+      });
+    };
     if (dataInterval.interval === "week") {
       // if tick interval is compressed then show months instead of weeks because they're nicer formatted
       const newTickInterval = computeTimeseriesTicksInterval(
         xDomain,
         tickInterval,
         chart.width(),
+        tickFormat,
       );
       if (
         newTickInterval.interval !== tickInterval.interval ||
@@ -138,21 +150,16 @@ export function applyChartTimeseriesXAxis(
       }
     }
 
-    chart.xAxis().tickFormat(timestamp => {
-      const { column, ...columnSettings } = chart.settings.column(
-        dimensionColumn,
-      );
-      return formatValue(timestamp, {
-        ...columnSettings,
-        column: { ...column, unit: tickFormatUnit },
-        type: "axis",
-        compact: chart.settings["graph.x_axis.axis_enabled"] === "compact",
-      });
-    });
+    chart.xAxis().tickFormat(tickFormat);
 
     // Compute a sane interval to display based on the data granularity, domain, and chart width
     tickInterval = {
-      ...computeTimeseriesTicksInterval(xDomain, tickInterval, chart.width()),
+      ...computeTimeseriesTicksInterval(
+        xDomain,
+        tickInterval,
+        chart.width(),
+        tickFormat,
+      ),
       timezone,
     };
   }

frontend/src/metabase/visualizations/lib/timeseries.js

@@ -178,13 +178,18 @@ function timeseriesTicksInterval(
   return TIMESERIES_INTERVALS[TIMESERIES_INTERVALS.length - 1];
 }
 
-/// return the maximum number of ticks to show for a timeseries chart of a given width. Unlike other chart types, this
-/// isn't smart enough to actually estimate how much space each tick will take. Instead the estimated with is
-/// hardcoded below.
-/// TODO - it would be nice to rework this a bit so we
-function maxTicksForChartWidth(chartWidth) {
-  const MIN_PIXELS_PER_TICK = 160;
-  return Math.floor(chartWidth / MIN_PIXELS_PER_TICK); // round down so we don't end up with too many ticks
+/// return the maximum number of ticks to show for a timeseries chart of a given width
+function maxTicksForChartWidth(chartWidth, tickFormat) {
+  const PIXELS_PER_CHARACTER = 7;
+  // if there isn't enough buffer, the labels are hidden in LineAreaBarPostRender
+  const TICK_BUFFER_PIXELS = 20;
+
+  // day of week and month names vary in length, but it's slow to check all of them
+  // as an approximation we just use a specific date which was long in my locale
+  const formattedValue = tickFormat(new Date(2019, 8, 4));
+  const pixelsPerTick =
+    formattedValue.length * PIXELS_PER_CHARACTER + TICK_BUFFER_PIXELS;
+  return Math.floor(chartWidth / pixelsPerTick); // round down so we don't end up with too many ticks
 }
 
 /// return the range, in milliseconds, of the xDomain. ("Range" in this sense refers to the total "width"" of the
@@ -197,11 +202,16 @@ function timeRangeMilliseconds(xDomain) {
 
 /// return the appropriate entry in TIMESERIES_INTERVALS for a given chart with domain, interval, and width.
 /// The entry is used to calculate how often a tick should be displayed for this chart (e.g. one tick every 5 minutes)
-export function computeTimeseriesTicksInterval(xDomain, xInterval, chartWidth) {
+export function computeTimeseriesTicksInterval(
+  xDomain,
+  xInterval,
+  chartWidth,
+  tickFormat,
+) {
   return timeseriesTicksInterval(
     xInterval,
     timeRangeMilliseconds(xDomain),
-    maxTicksForChartWidth(chartWidth),
+    maxTicksForChartWidth(chartWidth, tickFormat),
   );
 }
 

frontend/test/metabase/visualizations/components/LineAreaBarRenderer.unit.spec.js

@@ -113,6 +113,10 @@ describe("LineAreaBarRenderer", () => {
       ["2020-02-16T00:00:00.000Z", 1],
       ["2020-02-23T00:00:00.000Z", 1],
       ["2020-03-01T00:00:00.000Z", 1],
+      ["2020-03-08T00:00:00.000Z", 1],
+      ["2020-03-15T00:00:00.000Z", 1],
+      ["2020-03-22T00:00:00.000Z", 1],
+      ["2020-03-29T00:00:00.000Z", 1],
     ];
 
     // column settings are cached based on name.
@@ -138,7 +142,12 @@ describe("LineAreaBarRenderer", () => {
     expect(formattedWeek).toEqual("January 5 – 11, 2020");
 
     const ticks = qsa(".axis.x .tick text").map(e => e.textContent);
-    expect(ticks).toEqual(["January, 2020", "February, 2020", "March, 2020"]);
+    expect(ticks).toEqual([
+      "January, 2020",
+      "February, 2020",
+      "March, 2020",
+      "April, 2020",
+    ]);
   });
 
   it("should use column settings for tick formatting and tooltips", () => {

frontend/test/metabase/visualizations/lib/timeseries.unit.spec.js

@@ -2,6 +2,7 @@ import {
   dimensionIsTimeseries,
   computeTimeseriesDataInverval,
   getTimezone,
+  computeTimeseriesTicksInterval,
 } from "metabase/visualizations/lib/timeseries";
 import { getVisualizationTransformed } from "metabase/visualizations";
 
@@ -171,4 +172,81 @@ describe("visualization.lib.timeseries", () => {
       expect(timezone).toBe("US/Eastern");
     });
   });
+
+  describe("computeTimeseriesTicksInterval", () => {
+    // computeTimeseriesTicksInterval just uses tickFormat to measure the character length of the current formatting style
+    const fakeTickFormat = () => "2020-01-01";
+    const TEST_CASES = [
+      // on a wide chart, 12 month ticks shouldn't be changed
+      [
+        {
+          xDomain: [new Date("2020-01-01"), new Date("2021-01-01")],
+          xInterval: { interval: "month", count: 1 },
+          chartWidth: 1920,
+          tickFormat: fakeTickFormat,
+        },
+        { expectedInterval: "month", expectedCount: 1 },
+      ],
+      // it should be bump to quarters on a narrower chart
+      [
+        {
+          xDomain: [new Date("2020-01-01"), new Date("2021-01-01")],
+          xInterval: { interval: "month", count: 1 },
+          chartWidth: 800,
+          tickFormat: fakeTickFormat,
+        },
+        { expectedInterval: "month", expectedCount: 3 },
+      ],
+      // even narrower and we should show yearly ticks
+      [
+        {
+          xDomain: [new Date("2020-01-01"), new Date("2021-01-01")],
+          xInterval: { interval: "month", count: 1 },
+          chartWidth: 300,
+          tickFormat: fakeTickFormat,
+        },
+        { expectedInterval: "year", expectedCount: 1 },
+      ],
+      // shouldn't move to a more granular interval than what was passed
+      [
+        {
+          xDomain: [new Date("2020-01-01"), new Date("2021-01-01")],
+          xInterval: { interval: "month", count: 3 },
+          chartWidth: 1920,
+          tickFormat: fakeTickFormat,
+        },
+        { expectedInterval: "month", expectedCount: 3 },
+      ],
+      // Long date formats should update the interval to have fewer ticks
+      [
+        {
+          xDomain: [new Date("2020-01-01"), new Date("2021-01-01")],
+          xInterval: { interval: "month", count: 1 },
+          chartWidth: 1920,
+          tickFormat: () =>
+            // thankfully no date format is actually this long
+            "The eigth day of July in the year of our Lord two thousand and ninteen",
+        },
+        { expectedInterval: "year", expectedCount: 1 },
+      ],
+    ];
+
+    TEST_CASES.map(
+      ([
+        { xDomain, xInterval, chartWidth, tickFormat },
+        { expectedInterval, expectedCount },
+      ]) => {
+        it("should return " + expectedCount + " " + expectedInterval, () => {
+          const { interval, count } = computeTimeseriesTicksInterval(
+            xDomain,
+            xInterval,
+            chartWidth,
+            tickFormat,
+          );
+          expect(interval).toBe(expectedInterval);
+          expect(count).toBe(expectedCount);
+        });
+      },
+    );
+  });
 });