diff --git a/AGENTS.md b/AGENTS.md index 96accb0..1bac370 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -177,7 +177,7 @@ In recorded mode, `fixtureData.js`'s `test` answers every `https://data.firefox. | Switching branches without reinstalling | `node_modules` can still hold another branch's packages. Run `npm install` after a branch switch before trusting any result. | | Gitignored `src/components/views/css/` | Fresh clone cannot resolve the CSS imports until `npm run build:css` runs. Never hand-edit `css/` — edit `styl/`. | | Renaming a landmark ID | Breaks Playwright selectors silently. | -| `g.chart-line path` in a Playwright locator | A population with one data point draws a zero-size line (shown as a dot, `g.chart-point circle`). To check a chart drew, use `g.chart-line path` with `.filter({ visible: true }).first()`. Match a line to its population by its stroke color against the legend entry's color, not by index: a population with no points keeps its legend entry but draws no line. | +| `g.chart-line path` in a Playwright locator | A population with one data point, or a point with gaps on both sides, draws a zero-size line piece (shown as a dot, `g.chart-point circle`). A line breaks wherever its population skips a date that another population on the chart has. To check a chart drew, use `g.chart-line path` with `.filter({ visible: true }).first()`. Match a line to its population by its stroke color against the legend entry's color, not by index: a population with no points keeps its legend entry but draws no line. | | Redrawing every chart on every resize event | Across the hardware dashboard's 11 charts, redrawing on each resize event froze the page for about a minute on live data (#409). `containers/ChartContainer.jsx` waits 150 ms after the last resize event; `tests/playwright/specs/fixtures/resize.spec.js` guards it. | | `build:version.json` | Needs `build/` to already exist and `git` on PATH; must follow `build:app`. | | `no-shadow` / `no-redeclare` with `builtinGlobals` | Naming a local `name`, `status`, `history`, `event` is a hard error. | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c93568..af7fa74 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -265,7 +265,7 @@ style indirection layer, so a failure points straight at the markup it cares abo | Renaming a landmark ID | Breaks Playwright selectors silently. | | `build:version.json` | Needs `build/` to already exist and `git` on PATH; must follow `build:app`. | | `no-shadow` / `no-redeclare` with `builtinGlobals` | Naming a local `name`, `status`, `history`, `event` is a hard error. | -| Observable Plot charts and `g.chart-line path` | A population with a single data point draws a zero-size line path, shown as a dot (`g.chart-point circle`) instead. To check that a chart drew a line, use `g.chart-line path` with `.filter({ visible: true }).first()`. A population with no points keeps its legend entry but draws no line, so match lines to legend entries by color, not by index. | +| Observable Plot charts and `g.chart-line path` | A population with a single data point, or a point with gaps on both sides, draws a zero-size line piece, shown as a dot (`g.chart-point circle`) instead. A line breaks wherever its population skips a date that another population on the chart has. To check that a chart drew a line, use `g.chart-line path` with `.filter({ visible: true }).first()`. A population with no points keeps its legend entry but draws no line, so match lines to legend entries by color, not by index. | Known rot, recorded for recognition only, not as a roadmap: `react-app-polyfill` and `babel-polyfill`, plus a `browserslist` of `>0.2%, not dead, not ie <= 10, not op_mini all`, were diff --git a/src/components/views/Chart.jsx b/src/components/views/Chart.jsx index c59563d..34988dd 100644 --- a/src/components/views/Chart.jsx +++ b/src/components/views/Chart.jsx @@ -58,6 +58,31 @@ function assignMarkerRows(markers, xToPixel) { }); } +// Add an empty point for each date a population skips between its first and +// last points, so its line breaks there instead of joining across the gap. A +// date counts as skipped when another population on the chart has it. +function withGaps(sortedPoints, chartDates, population) { + if (!sortedPoints.length) return []; + + const pointsByDate = new Map(sortedPoints.map(point => [+point.x, point])); + const first = +sortedPoints[0].x; + const last = +sortedPoints.at(-1).x; + + return chartDates + .filter(date => date >= first && date <= last) + .map(date => ({ ...(pointsByDate.get(date) || { x: new Date(date), y: null }), population })); +} + +// A point with a gap or the end of its line on both sides has no line through +// it, so it needs drawing on its own. +function isolatedPoints(pointsWithGaps) { + return pointsWithGaps.filter((point, index) => ( + point.y !== null + && (pointsWithGaps[index - 1]?.y ?? null) === null + && (pointsWithGaps[index + 1]?.y ?? null) === null + )); +} + export default props => { // Units that can appear right after a value, without a space in between const spacelessUnits = ['%']; @@ -86,8 +111,14 @@ export default props => { return

No data to show for this metric.

; } - const singlePoints = props.data.flatMap((population, index) => ( - population.length === 1 ? [{ ...population[0], population: legend[index] }] : [] + const chartDates = [...new Set(points.map(point => +point.x))].sort((dateA, dateB) => dateA - dateB); + const linePoints = legend.flatMap(populationName => withGaps( + points.filter(point => point.population === populationName), + chartDates, + populationName + )); + const loosePoints = legend.flatMap(populationName => isolatedPoints( + linePoints.filter(point => point.population === populationName) )); // The minimum and maximum y values among all populations. @@ -195,7 +226,7 @@ export default props => { // A chart with a single population is shaded below its line. const maybeArea = props.data.length === 1 ? [ - Plot.areaY(points, { x: 'x', y1: minYToShow, y2: 'y', curve: 'catmull-rom', fill: '#0000ff', fillOpacity: .2, className: 'chart-area' }), + Plot.areaY(linePoints, { x: 'x', y1: minYToShow, y2: 'y', curve: 'catmull-rom', fill: '#0000ff', fillOpacity: .2, className: 'chart-area' }), ] : []; const options = { @@ -217,8 +248,8 @@ export default props => { Plot.ruleX(markersInRange, { x: 'x', stroke: '#b3b2b2', strokeDasharray: '3,1', className: 'chart-marker' }), ...markerLabelMarks, ...maybeArea, - Plot.line(points, { x: 'x', y: 'y', z: 'population', stroke: 'population', strokeWidth: 1, curve: 'catmull-rom', className: 'chart-line' }), - Plot.dot(singlePoints, { x: 'x', y: 'y', fill: 'population', r: 3, className: 'chart-point' }), + Plot.line(linePoints, { x: 'x', y: 'y', z: 'population', stroke: 'population', strokeWidth: 1, curve: 'catmull-rom', className: 'chart-line' }), + Plot.dot(loosePoints, { x: 'x', y: 'y', fill: 'population', r: 3, className: 'chart-point' }), Plot.dot(points, Plot.pointer({ x: 'x', y: 'y', fill: 'population', r: 4, maxRadius: hoverRadius, className: 'chart-hover-point' })), ], }; diff --git a/src/tests/jest/Chart.test.jsx b/src/tests/jest/Chart.test.jsx index f84f8f8..3c0a248 100644 --- a/src/tests/jest/Chart.test.jsx +++ b/src/tests/jest/Chart.test.jsx @@ -259,3 +259,64 @@ it('The y and x axes are drawn as grey lines along the left and bottom of the ch expect(left.getAttribute('x1')).toBe(left.getAttribute('x2')); expect(bottom.getAttribute('y1')).toBe(bottom.getAttribute('y2')); }); + +const weeks = ['2026-01-04', '2026-01-11', '2026-01-18', '2026-01-25', '2026-02-01', '2026-02-08'].map(date => new Date(date)); + +// Counts the visible pieces of a line. Plot writes an isolated point as a +// lone move-to, which draws nothing, so only pieces that go somewhere count. +function lineSegments(svg, stroke) { + const path = [...svg.querySelectorAll('g.chart-line path')].find(line => line.getAttribute('stroke') === stroke); + return path.getAttribute('d').split('M').filter(piece => /[LC]/.test(piece)).length; +} + +it('A population\'s line breaks across weeks it is missing from, when other populations have those weeks', () => { + const svg = renderChart({ + data: [ + weeks.map((x, index) => ({ x, y: 10 + index })), + [weeks[0], weeks[1], weeks[4], weeks[5]].map(x => ({ x, y: 5 })), + ], + }).svg; + + expect(lineSegments(svg, '#4040e8')).toBe(1); + expect(lineSegments(svg, '#05b378')).toBe(2); + expect(svg.querySelectorAll('g.chart-point circle')).toHaveLength(0); +}); + +it('A point with a gap on both sides is drawn as a dot', () => { + const svg = renderChart({ + data: [ + weeks.map((x, index) => ({ x, y: 10 + index })), + [weeks[0], weeks[1], weeks[3], weeks[5]].map(x => ({ x, y: 5 })), + ], + }).svg; + + expect(lineSegments(svg, '#05b378')).toBe(1); + const dots = [...svg.querySelectorAll('g.chart-point circle')]; + expect(dots).toHaveLength(2); + dots.forEach(dot => expect(dot.getAttribute('fill')).toBe('#05b378')); +}); + +it('A week that no population has does not break the lines', () => { + const everyWeekButOne = weeks.filter((x, index) => index !== 2); + const svg = renderChart({ + data: [ + everyWeekButOne.map((x, index) => ({ x, y: 10 + index })), + everyWeekButOne.map(x => ({ x, y: 5 })), + ], + }).svg; + + expect(lineSegments(svg, '#4040e8')).toBe(1); + expect(lineSegments(svg, '#05b378')).toBe(1); +}); + +it('A population that starts later than the others is drawn from its first point, with no gap before it', () => { + const svg = renderChart({ + data: [ + weeks.map((x, index) => ({ x, y: 10 + index })), + weeks.slice(3).map(x => ({ x, y: 5 })), + ], + }).svg; + + expect(lineSegments(svg, '#05b378')).toBe(1); + expect(svg.querySelectorAll('g.chart-point circle')).toHaveLength(0); +}); diff --git a/tests/playwright/specs/fixtures/chart-series.spec.js b/tests/playwright/specs/fixtures/chart-series.spec.js index 5be37b2..193f92d 100644 --- a/tests/playwright/specs/fixtures/chart-series.spec.js +++ b/tests/playwright/specs/fixtures/chart-series.spec.js @@ -113,6 +113,28 @@ test('A line is drawn in date order even when its data is not', async ({ page }) readoutDates.slice(1).forEach((date, index) => expect(date).toBeGreaterThan(readoutDates[index])); }); +test('A line breaks across weeks its population is missing from, and a point left on its own is a dot', async ({ page }) => { + // Oldest first. Sparse is missing two runs of weeks, leaving one week on + // its own between them. + const oldestFirst = dashboard.dates.toSorted(); + const sparseWeeks = oldestFirst.filter((date, index) => ![3, 4, 6, 7].includes(index)); + await reloadWithFirstMetric(page, { + Steady: oldestFirst.map(date => ({ x: date, y: 300 })), + Sparse: sparseWeeks.map(date => ({ x: date, y: 100 })), + }); + const metricOverview = page.locator('#metric-overview-1'); + const sparseColor = await metricOverview.locator('.legend .legend-entry').filter({ hasText: 'Sparse' }) + .evaluate(element => getComputedStyle(element).color); + + const sparseLine = await lineWithColor(metricOverview, sparseColor); + const visiblePieces = (await sparseLine.getAttribute('d')).split('M').filter(piece => /[LC]/.test(piece)); + expect(visiblePieces, 'the line is drawn in two pieces, either side of the gaps').toHaveLength(2); + + const dot = metricOverview.locator('g.chart-point circle'); + await expect(dot).toHaveCount(1); + expect(await dot.evaluate(element => getComputedStyle(element).fill)).toBe(sparseColor); +}); + test.describe('On a narrow screen', () => { test.use({ viewport: { width: 375, height: 800 } });