improvement(charts): a Heatmap composed from existing primitives - #1219
Conversation
One row per entity, one column per time slot, each cell coloured by its value and describing itself on hover or focus. It is assembled from Box, Tooltip, Text and ChartLegend rather than from a charting library: the grid is CSS, so it costs no Recharts instance and scales to a few hundred cells without one. The component owns everything it draws — title, row labels, grid, x-axis and legend — so an app hands it data and a colour scale rather than a composition. Two scales, and they type the data with them. `discrete` — the default — takes a colorSet and mounts its own ChartLegendWrapper, so the legend both colours the cells and filters the grid on click; omitting the colorSet reads a wrapper the caller put above instead, which is how several charts come to share one legend. `continuous` ramps one colour by opacity and stands beside a HeatmapGradientScale handed the same max the cells were ramped against, so the numbers printed by the scale cannot drift from what the grid shows. The grid is driven by `columns`, not by each row's cells. A row shorter than the axis has to leave the rest of its line empty, and CSS auto-placement does the opposite — it pulls the next row's label out of the gutter and skews everything below it. A `null` cell holds its slot. Three smaller decisions worth knowing. Colours are resolved once per distinct value rather than once per cell, because a dense grid asks `getColor` the same four questions four hundred times and each miss warns. Cell opacity is rounded to two decimals, which is past the eye's resolution and stops a dense grid from minting a styled class per cell. And the prop is `scale`, not `colorScale`: Storybook infers a colour picker for any prop whose name matches /color/i. The stories keep the data and a useStatusScale() that says what OK, WARNING, CRITICAL and NONE are worth in the theme's colours — that much is app domain, and the component has no business knowing those statuses.
Hello clement-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
…status The stories only ever showed OK / WARNING / CRITICAL / NONE in the theme's status tokens, which reads as the set of values a Heatmap supports rather than as one example of them. Three stories say otherwise. CustomColorSet takes five backup outcomes and colours them half from the chart palette and half from the theme's own tokens, because colorSet takes any CSS colour and there is no reason to pick them all from one place. NonStatusValues drops the theme entirely: four workload profiles in four hand-written hex values, nothing about health in them. LabelledValues puts bare response codes in the cells, then hands labelMap to the legend and formatValue to the tooltip and aria-label, so the data keeps its codes and the reader gets sentences; its sortOrder compares them as numbers. Generalising the data helper is what made the three cheap. buildCategoryRows fills a grid over any value set, weighting each value at half the frequency of the one before it — an even wash hides which colour means what, which is the only thing these stories are about. inDeclaredOrder replaces the indexOf-with-cast comparator that useStatusScale had inline.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Hello Clément, I pulled the branch and ran the storybook locally, so this is feedback from using it, not from reading the code. First, the thing I want us to be explicit about: in a heatmap, colour is the only channel carrying information. By definition it is not accessible on its own, and here the tooltip is what saves it. I am fine with that for this component, it is the very nature of a heatmap, but two consequences follow. The limitation has to be written down in the guidelines rather than discovered, and I want to keep the number of components that rely on colour alone as small as possible. So this one should not become a habit. What I like: the row labels are right-aligned, which makes them much easier to scan against the grid. The selected state is a good call too, it uses the design system's selection token and it is painted as an outline, so nothing moves when you hover. Accessibility is clearly not an afterthought either, every cell is reachable and announces itself, and the tooltip opens on focus, not only on hover. Now the points I would like you to look at.
|
Cuervino
left a comment
Cuervino
left a comment
There was a problem hiding this comment.
Requesting changes so this does not land before we settle the design points, which I left in full in the comment above: #1219 (comment)
Happy to discuss any of them, several are questions rather than asks.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following reviewers are expecting changes from the author, or must review again: |
|
Even if my previous content looked AI-generated, it's really my personal review, but maybe the style is not understandable enough because generated by Claude. Please tell me if it's the case, I need to adapt and to have the correct tone, and it should be explainable enough. |
A cell now says how long it lasts. It only ever gave the instant its column opened, so reading it meant measuring it against the next tick to find out whether the square covered five minutes, an hour or a day. The duration is read off the axis — a column lasts until the next one starts, the last reuses the gap before it — so an irregular axis is followed rather than assumed away, and no new prop is needed. The end repeats the date whenever the slot changes day: without that, a nightly slot read "31 Aug 23:00 to 00:00" and a daily one "31 Aug 00:00 to 00:00", the same instant twice as far as the reader could tell. getDateDaysDiff cannot answer that question, being an elapsed duration where this is a calendar one, hence isSameCalendarDay. labelWidth promised that labels truncate rather than widen the gutter, and nothing in the CSS made them. They overflowed onto the grid instead. The gutter clips now, with min-width because a grid item defaults to the width of its content and would otherwise refuse to shrink into its track, and carries the full label in a title so the truncation loses nothing. Cells no longer show a pointer cursor. Nothing in the grid is clickable and the hand promised an action that does not exist. Colour sets stop crossing. Status tokens are for states of health; everything else — job outcomes, workload profiles — takes the series colours, because a value painted statusHealthy claims to be healthy and a backup type is not. One story had four hex values written into it, which belong to no theme and follow none. Worth knowing: the series colours are module constants, identical in all four themes, so they are the design system's categorical palette rather than a theme-reactive one. The axis speaks the date guideline. It printed "09-02", which is 2 September or 9 February depending on the reader; day-month-abbreviated gives "02 Sep". The component's own default tick is still time-of-day and has no notion of its axis granularity, so a daily axis without an explicit formatColumnTick would print midnight fourteen times. Stories stop shouting: OK / WARNING / CRITICAL / NONE became Ok / Warning / Critical / No data, since stories are what people copy. Periods leave the titles, a range being something a global selector owns rather than a chart. A story crossing midnight puts on the record that the axis says nothing when the day changes. And Numeric Values gained a toggle between a ramp pinned to 100 and one topped by the data, with the sample load lowered to around 60% so the difference is visible — two grids whose ramps top out at their own maxima are not comparable. The guideline page is a first version, and deliberately silent on the continuous scale while its fate is still being discussed. It states the limitation the component cannot design away: colour is the only channel carrying information, the tooltip is what saves it, and that is a reason to keep such components rare rather than a pattern to copy.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following reviewers are expecting changes from the author, or must review again: |
The Labelled Values story still painted 200 with statusHealthy, 403 with statusWarning and 500 with statusCritical, which is the crossing the rest of the review already removed everywhere else. A response code looks like a health status and is not one. A 403 is the server working exactly as asked; a 500 in one endpoint's row says nothing about whether the product is degraded. Painting them with the status tokens makes every red cell in the product claim the same thing whether it holds or not, and that claim is the whole value of those three colours where they do apply. Four series colours instead, none of them crimson, so the reading order of the legend comes from sortOrder comparing the codes as numbers rather than from a hue quietly ranking them.
There was a problem hiding this comment.
Hello Clément,
I ran the storybook on 7267bcf1, most of what I raised is in. Thanks, and good call on the response codes.
I am approving so this is not stuck on me, on the understanding that the continuous scale goes before merge. See my comment on next.ts.
One question (it is what made me rework the guideline): every cell is an aggregate, and the rule behind it changes the picture: worst value wins, average and last value give three different grids from the same data.
👉 Which rule should we recommend ? Do you think the component should help state it to the reader?
The rest is on the lines.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
…is tick The continuous scale goes. No product needs it today, and a published prop nobody exercises still has to be documented, tested and supported. The day intensity is actually needed, the use case will say whether it wants a ramp or a few steps, which is a better place to decide it from than this one. Out with it: HeatmapGradientScale, HeatmapContinuousScale, ContinuousHeatmapProps, getHeatmapMaxValue, getRampOpacity, DEFAULT_MIN_OPACITY, the Numeric Values story and the exports of all of it. Two open questions close with it — pinning the top of a percentage ramp to 100, and the gradient bar starting at minOpacity while its bottom label said min, so it showed shades no cell would ever use. What is left is simpler than what it replaced. HeatmapProps stops being a union, so Heatmap has neither a branch nor the casts that branch needed, and the value type narrows from string | number to string — a heatmap value is a name, and HeatmapRow<CellStatus> still lets a caller say which names. formatValue keeps its test: it was only ever covered through the continuous scale, and it is a discrete prop too. The x-axis now works out its own tick format. It was hardcoded to the time of day and ignored the step of the axis, so a daily grid printed "00:00" over every one of its columns unless the caller passed formatColumnTick. The slot duration is already known from getColumnEnds, so the default reads it: time of day below a day, the abbreviated date from a day up. The date guideline then lives in the component rather than in every caller, which is why the guideline page no longer states it. That guideline page is Cuervino's rewrite, kept whole. It adds the two things mine was missing. First, what a cell hides: it is an aggregate, worst-value-wins and average and last-value produce three different grids from the same data, and the reader cannot tell which rule made the picture — so the page has to ask for it to be stated. Second, the name, since a heatmap in UX research is clicks or gaze painted over a screenshot, an unrelated object that shares the word. Titles go to sentence case throughout, because examples are what people copy and the rest of the charts in that file already were.
| gridTemplateColumns={`${labelWidth} repeat(${Math.max( | ||
| columns.length, | ||
| 1, | ||
| )}, minmax(0, 1fr))`} |
There was a problem hiding this comment.
No min on cell, meaning that if the column number increase or if available space is small, the cell will not stop shrinking. Also the labels for time can overlap.
@Cuervino Should the cell have a min size ? And what happens if there is no enough space ?
There was a problem hiding this comment.
A new Horizontal Scroll story is available to try this, giving each cell a minimum width so a long axis scrolls sideways instead of shrinking.
There was a problem hiding this comment.
Yes, it's a very good question, @JeanMarcMilletScality.
No minimum size that makes the grid scroll, as a status history is read as one picture, so the window has to fit.
As is, this component might be tricky, especially if it doesn't calculate anything regarding how much space it has, but only depends on the backend.
And maybe you (Jean-Marc) need to provide a bit of responsiveness rules here since it's a topic that you start to be an expert at.
It might be possible to set the legend on the bottom and not on the right if there is not enough space, and also maybe there is a way to combine and to make a new aggregation if there is not enough space to reduce the number of horizontal slots.
…croll Four things Jean-Marc asked for. labelWidth was typed string and interpolated into grid-template-columns, where a length CSS cannot parse does not fail alone — it invalidates the whole declaration, repeat() included, and the grid collapses. It is a CSS length now, and it no longer goes near the template: the gutter cell carries its own width, so a bad value costs one label. cellHeight, cellGap and cellMinWidth take the same type, and each says what it expects and what it defaults to. sortOrder and labelMap are read only alongside colorSet — without one there is no legend of ours to order, and the ChartLegendWrapper the caller put above owns it. That was true and unwritten; it is written on both props now. A cell announced itself as its row and its value, leaving a screen reader to count columns to find out when it happened. It carries its slot too, from the same formatSlot the tooltip uses, so the two cannot drift. And the grid says it is one: role="grid", rows, gridcells — the empty ones included, or the columns stop lining up for anyone stepping through them. Then the part that was not asked for as such. Columns share the width available, so a long axis in a narrow frame kept dividing until the cells were slivers a couple of pixels wide: present, aligned, and saying nothing. cellMinWidth is a floor, 12px by default, and past it the grid scrolls sideways instead. Scrolling is what made the rest of the layout move. The labels sit outside the scrolling area rather than pinned inside it, which is worth spelling out because pinning is the obvious thing to try and it costs three problems: something opaque to paint, therefore a background the component has to be told about, therefore a hover ring bleeding past the box that paints it. Outside, they simply do not move, and the scrollbar covers the tiles alone rather than promising that the labels move too. Rows became subgrid to keep the two columns level — the scroller takes the rows it is placed in rather than sizing its own — and overflow-y is hidden, because a horizontal scrollbar shortens the box by just enough to summon a vertical one. The row labels lost role="rowheader" on the way out of the grid: a rowheader has to be owned by the row holding its cells. Each row carries an aria-label instead, each cell still names its row, and the visible column is aria-hidden so none of it is announced twice.
The page already asks the reader to decide how wide a slot is, and to say which rule aggregated it. It said nothing about how many of those slots the frame can hold, which is the decision that comes next and the one the component now has an opinion about. It sits under the aggregation section rather than beside the time axis, because it is the same question: slot width decides what a cell hides, the floor decides how much of the grid you can see without scrolling for it. Raising the floor to give each slot more room trades one way of losing the pattern for another.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
…e its cells A narrowing frame used to take it out of the cells alone: the label gutter held a fixed `labelWidth` and the legend held its column, so the tiles were the only thing left to squeeze, and they reached `cellMinWidth` and scrolled while both neighbours still had room to give. Reverse the order, cheapest first. The row labels truncate — they keep the whole name in a tooltip, so nothing is lost. The legend then drops under the grid and turns horizontal, which costs nothing at all. Only then do the cells narrow, and past their floor the grid scrolls as before. `labelWidth` becomes a cap rather than a width, so the gutter also stops leaving dead space beside short labels. Two lengths carry the policy: `LABEL_MIN_WIDTH`, how far the labels give way, and `PREFERRED_CELL_WIDTH`, the cell width they give way to protect — measured against `cellMinWidth` instead, the cells would always reach the floor first and the gutter would only yield once scrolling had started. Which way the legend went is measured rather than predicted: the break depends on how many columns the axis has, so there is no breakpoint to write. Reading it back cannot oscillate, since going under only makes the legend wider. Also fixes two things the responsive work made visible: an x-axis tick wrapped to two lines inside its single-column cell, and the title was a size and a weight above the `ChartTitle` that `ChartHeader` gives every other chart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Tooltip does not wrap its children transparently: it puts a container and a reference div around them. The role sat on the Cell, two levels under, so what each row actually owned was an anonymous div and the gridcells were buried beneath it — the one arrangement ARIA does not allow, since a row's required owned elements are its cells. The role moves to a wrapper above the Tooltip. That wrapper is the grid item now, which is why it is display: grid — TooltipContainer is inline-block, and behind an ordinary block it would shrink to fit a cell that has no width of its own, collapsing every tile to nothing. The gap that let this through is worth as much as the fix. The suite counted six gridcells and found them by name, and both stayed true with the cells buried, because neither says anything about who owns them. There is a test for the ownership itself now; it fails on the old shape, with the three nulls of the intervening divs. Empty cells were always correct, having no tooltip to be wrapped in — which is how the grid came to be right for its holes and wrong for every painted cell.
… ring A cell paints its hover and focus ring outside its own box, and the scroller clips at its padding box, so the ring on any cell along an edge was cut — top and bottom by the hidden vertical overflow, the leading and trailing columns by the horizontal one. Padding gives it that room, and gives the outermost tiles a little air from the gutter and the frame besides.
The page named sortOrder and described only the default, which left a reader to find the other two in the type. Documenting the status shorthand honestly turned out to be the whole point. 'status' does not sort. It returns ['Success', 'Warning', 'Failed'] filtered by what the grid happens to hold, so it is a whitelist: any value named otherwise keeps its colour on the cells and loses its legend entry, which leaves it neither readable nor filterable on a grid whose own guideline says two sections earlier that colour is the only channel. That catches the absence value as well, and a status history always has one, since collection has gaps. So the shorthand is not "for grids that spell their values that way", it is for charts with exactly three outcomes and no fourth. Heatmaps take the comparator.
ca3fa40 to
049efb3
Compare
…ries it The guideline had grown a paragraph where it wanted a list item: three sort orders, one of them arguing its case at four times the length of the other two. The page states what 'status' keeps and what it leaves out, and stops there. The argument belongs on the prop, where a caller meets it while choosing rather than while reading a design page, so sortOrder's own doc now carries the three forms and what the shorthand really does: a whitelist, not a sort, and a value it drops keeps painting its cells — left coloured, unexplained and impossible to filter. One thing worth stating there that was not written anywhere: a comparator is handed the colorSet keys, not the values behind them. With labelMap in play it is easy to assume you are ordering what the legend displays, when you are ordering what the cells hold.
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
Please check the status of the associated issue None. Goodbye clement-scality. |
4 participants
Component:
charts— a newHeatmapTL;DR — Adds a
Heatmapcomponent: one row per entity, one column per time slot, each cell coloured by its value.Split out of #1190, which carried this plus a logarithmic Y axis. The axis is in #1218 now; this one is the heatmap and nothing else.
Context / Why
Nothing in
core-uireads one metric across many entities over time — the shape a service-status board or a per-node health strip wants. It started life in #1190 as a story-only recipe, a design proposal to be argued with before anything landed insrc/. That review happened, the shape held, so it is a component.It is assembled from
Box,Tooltip,TextandChartLegendrather than from a charting library: the grid is CSS, so it costs no Recharts instance and scales to a few hundred cells without one.🧩 Approach
The component owns everything it draws — title, row labels, grid, x-axis, legend — so an app hands it data and a colour scale, not a composition:
Two scales, and they type the data with them:
scalediscrete(default)ChartLegendWrapperfromcolorSet, so the legend both colours the cells and filters the grid on click. OmitcolorSetto read a wrapper the caller put above instead — that is how several charts share one legend.continuousHeatmapGradientScalehanded the samemaxthe cells were ramped against, so the printed domain cannot drift from what the grid shows.The grid is driven by
columns, not by each row's cells. A row shorter than the axis has to leave the rest of its line empty, and CSS auto-placement does the opposite — it pulls the next row's label out of the gutter and skews everything below it. Anullcell holds its slot.What stays with the caller is domain, not composition: which colour
OK/WARNING/CRITICAL/NONEare worth, and in what order the legend lists them. The component has no business knowing those statuses.📷 Screenshots
ScreenshotEquivalent— 5 services, 4 columns, the last one with no data yet. The rest of the stories are worth clicking through rather than screenshotting.🔍 Review focus
charts/heatmap/Heatmap.tsx— the props surface, and two calls worth a second opinion. One implementation per scale rather than one branch inside it, because the discrete scale reads the legend context and a hook cannot be called conditionally; the dispatcher pays for it with a cast, since TypeScript narrows on a top-level discriminant and not onscale.typeone level down. AndshowLegenddefaults totrueeven when the wrapper is inherited, so two heatmaps under one shared wrapper draw two identical legends until the caller says otherwise — a predictable default preferred over an implicit rule.charts/heatmap/Heatmap.utils.ts— the opacity ramp: clamped to its domain, and rounded to two decimals so a dense grid does not mint a styled class per cell.Tooltipopens on focus). On theDense gridstory that is ~400 tab stops. Say so if you would rather trade the keyboard path for a shorter tab order.🧪 How to test
Charts → Heatmap. It opens on a
Playgroundwhose data is a control, so it is meant to be poked at rather than read:Playground→ Controls →rows. It is a JSON editor: add a row, rename one, change any cell toOK/WARNING/CRITICAL/NONE. The x-axis is derived from the longest row, so adding cells adds columns.rows: delete the last cells of one row. Its line ends early and every other row stays put — that is the auto-placement fix.Dense grid→ pushcellGapto 0 for a continuous timeline, then raisecolumnsto 96 and uselabelEveryto keep the axis legible.noDataColumnsis the "collection has not caught up" tail.Numeric valuesis the continuous scale:minOpacityis the floor that keeps the low end visible, and the gradient beside the grid states the domain the cells were ramped against.Theme switching is worth a pass: the status colours, the cell outlines and the gradient all come from tokens.
npm test -- src/lib/components/charts/heatmap🔗 References
Breaking Changes: none.
Heatmap,HeatmapGradientScaleandgetHeatmapMaxValueare new exports onnext.ts; nothing existing changes.No issue or ticket for this one.
What changed
charts/heatmap/is the component:Heatmap.tsx(the frame, the grid and one implementation per scale),HeatmapGradientScale.tsx(the continuous legend, placed by the component or by the caller, likeChartLegend),Heatmap.utils.ts(the ramp and the domain) and their tests.stories/Heatmap/keeps the data and the theme colours, and nothing else.Two shapes were tried and dropped along the way in #1190. One stacked a
GlobalHealthBarper row — it worked, but it needs one chart instance per row and hides every x-axis but the last. The other was aNestedProportionBar, a different chart altogether, which is not part of this PR.