Charts: add HeatmapChart (CHARTS-218)#49855
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
|
@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself. |
Code Coverage Summary7 files are newly checked for coverage. Only the first 5 are listed here.
|
Here's my review of the HeatmapChart PR. All feedback is purely observational — no code changes made. 🔴 Missing public type exports
Any TypeScript consumer who uses 🔴 Keyboard navigation is 1D on a
|
573c65d to
401c416
Compare
|
@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself. |
The only change since the previous review is the pnpm-lock.yaml regeneration ( The heatmap source is otherwise unchanged. All items from the previous review still stand: 🔴 Still open:
🟡 Still open:
🔵 Still open:
|
|
Thanks @copilot — assessed all eight. Addressed in
Two I'm intentionally leaving, with reasoning:
Re-requesting your review of the above. |
|
@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself. |
All items from the previous two rounds are resolved in ✅ Previously raised items — all resolved
🟡 Dead
|
All six items from the previous review are addressed in 🟡
|
|
@claude please review this PR. Copilot's rounds are complete — all its findings were addressed (2D keyboard nav + ARIA grid/row hierarchy, root type exports, |
|
Claude finished @adamwoodnz's task in 37s —— View job Re-review: HeatmapChart (CHARTS-218) —
|
| Item | Verified |
|---|---|
| 🟡 First arrow press lands on (0,0) | ✅ heatmap-chart.tsx:135-138 — if ( selectedIndex === undefined ) { setSelectedIndex( 0 ); return; } short-circuits before the move logic, so the first press enters onto cell 0 and subsequent presses move from it (WAI-ARIA grid entry). |
| 🟡 Legend spans the data colors | ✅ heatmap-legend.tsx:36 — every swatch is now colorFor( value ); the index === 0 ? emptyColorHex special-case is gone, so the gradient runs colorFor(min)→colorFor(max) and the lightest data color (lightColorHex) is the first swatch. Matches the cells. |
| 🔵 Ragged-column guard | ✅ heatmap-chart.tsx:347-349 — if ( ! cell ) { return null; } guards before the cell.bin as HeatmapCell access, so unequal-length columns can't throw on undefined. |
🔵 Unused chartRef |
✅ Gone — declaration, attribute, and the useRef import all removed (heatmap-chart.tsx:7 now imports only createContext, useCallback, useContext, useMemo, useState). |
Two small consistency notes while I was in the file — both pre-existing, neither blocking:
- The flat-index encoding is consistent end-to-end now: keyboard nav uses
col * rows + row(:153),aria-activedescendantdecodes with the samerowsdivisor (:279-284), and the cell renderer computescell.column * rows + cell.row(:352) — all keyed off the globalrows = Math.max(0, …)(:105), so the ragged-column guard and the nav math agree. getCountstill mapsnull→0(:51-52), which remains inert since nocolorScale/opacityScaleis passed toHeatmapRect— exactly as you reasoned earlier. No action needed.
Nothing further from me — this is ship-ready. Re: rebase, agreed it's the clean final step; I can't perform branch operations from here, so a manual rebase onto trunk before merge is the path.
· charts-218-heatmap-chart
Review cycle complete ✅Final reviewer: @claude (Copilot rounds complete first). Addressed across the cycle:
CI: green / pending (no failures); the one earlier failure (lock files) is fixed. Status: clean — ready for human review and the merge decision. Note: the branch is MERGEABLE but a few commits behind |
|
Claude finished @adamwoodnz's task in 46s —— View job Final verification ✅I re-read the current branch and confirmed every item from both review cycles is genuinely resolved in the code — not just claimed.
One side-effect worth flagging (not a blocker): dropping the Status: I concur — clean and ready for human review + the merge decision. No open addressable items from either review round. Rebase: Confirmed I can't perform branch operations (merge/rebase) from here. The branch is |
6f25ce1 to
5c59dc6
Compare
…CHARTS-218) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…legend steps guard, docs (CHARTS-218) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sition legend fits below it (CHARTS-218) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…CHARTS-218) The lockfile committed earlier was generated through a local registry proxy, which records explicit tarball: URLs that a default-registry install (CI) does not, failing the 'Lock files are up to date' check. Regenerated against the npmjs registry so only the @visx/heatmap entry is added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hierarchy, root type exports, dedupe (CHARTS-218) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s handler, add arrow-key tests (CHARTS-218) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…0,0), legend spans data colors, ragged-column guard, drop unused ref (CHARTS-218) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Compact mode now sizes cells to the smaller of the per-column/per-row fit so they render as squares (matching the contribution-graph design) instead of stretching to fill. Non-compact still fills the area. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e title tooltip (CHARTS-218) The hover tooltip used raw clientX/clientY, but TooltipInPortal re-adds the container offset, placing it far from the cursor — subtract containerBounds so it lands at the pointer. Replace the per-cell SVG <title> (which also triggered a native browser tooltip) with aria-label, keeping the accessible name without the duplicate tooltip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…RTS-218) Compact used a 14px top margin, too small for the 12px column label rendered at y=-6, clipping it at the SVG top edge. The label band is the same size in compact (only the cells shrink), so use a 20px top margin for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…CHARTS-218) Replaces the ad-hoc empty-state div with the Center layout primitive (from CHARTS-212 / #49164) so the 'No data available' message is centred on both axes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5c59dc6 to
ddba57e
Compare
Fixes CHARTS-218
Why
@automattic/chartshad no way to visualize a two-dimensional matrix of values on a color scale. CHARTS-218 needs a heatmap — in particular a GitHub-contribution-style "Posting Activity" calendar — for product widgets, including a compact variant for short widgets. This adds that chart type using the same theming, accessibility, and composition patterns as the rest of the library, so it drops into existing consumers without new conventions.Proposed changes
HeatmapChart(custom-SVG over@visx/heatmap'sHeatmapRect).data: HeatmapColumn[](columns of cells) plus optionalrowLabels;nullvalues render as empty cells.GeoChart's approach: ascaleLinearfrom a lightened theme primary (lightenHexColor(primary, 0.8)) to the full theme primary across the value extent, with the primary resolved to hex viagetElementStyles+normalizeColorToHex/resolveCssVariable. Empty cells use the greyvar(--jp-gray-0, #f6f7f7)fill.compactmode — square cells, hides in-cell values, tightens the cell gap, and thins axis labels.<HeatmapChart.Legend />renders aLess … Morecolor-scale swatch row (composition-only; noshowLegend/legendprops).buildCalendarHeatmapData()— a pure helper that buckets a flatDataPointDate[]series into weekday-row × week-column data with month labels, for the contribution-calendar layout.role="grid"with agrid › row › gridcellhierarchy, per-cellaria-labelaccessible names, full 2D arrow-key navigation, and hover tooltips (@visx/tooltip).Centerlayout primitive (CHARTS-212)..stories.tsx+.docs.mdx+.api.mdx), the library Introduction catalog entry, and the README import example.Screenshots
Default — matrix with month/quarter top labels, weekday rows, value-scaled cells, in-cell numbers.
Compact — square cells with values hidden, for short widgets.
Calendar — GitHub-contribution-style layout via
buildCalendarHeatmapData.With composition legend — the
Less … Morescale renders below the chart.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This adds a presentational chart component; it collects and tracks nothing.
Testing instructions
projects/js-packages/charts, runpnpm run typecheck(clean) andpnpm run test(911 passing — chart render/labels/values/compact, tooltips + a11y, legend, color utilities, calendar helper).pnpm run storybookand open JS Packages / Charts Library / Charts / Heatmap Chart:Less … Morelegend renders below the chart.