fix: improved heatmap legend #1317
Conversation
markov00
added
:legend
packages/charts/package.json
Outdated
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "@elastic/charts", | |||
| "description": "Elastic-Charts data visualization library", | |||
| "version": "34.0.0", | |||
| "version": "34.0.0a", | |||
There was a problem hiding this comment.
| "version": "34.0.0a", | |
| "version": "34.0.0", |
storybook/stories/heatmap/3_time.tsx
Outdated
| bands: [ | ||
| { color: '#ca0020', start: -5, end: -3 }, | ||
| { color: '#f4a582', start: -3, end: -1 }, | ||
| { color: 'rgb(206,206,206)', start: -1, end: 1 }, |
| // filter out wrongly configured bands | ||
| if (start > end) { | ||
| return acc; | ||
| } | ||
| acc.push({ start, end, color, label: label ?? labelFormatter(start, end) }); | ||
| return acc; |
There was a problem hiding this comment.
instead of the double return, would like the positive
// admit only proper bands
if(start < end) acc.push({ start, end, color, label: label ?? labelFormatter(start, end) });
return acc;
| const scale = getBandScale(bands); | ||
| return { scale, bands }; |
There was a problem hiding this comment.
Instead of cramming in the scale too, thus turning a simpler return type into a deeper tuple (object), the bands could be computed separately where getBandsColorScale is also used (lower coupling)
There was a problem hiding this comment.
linking your next comment #1317 (comment)
| function getBandScale(bands: ColorBand[]): ColorScale { | ||
| return (value) => { | ||
| for (let i = 0; i < bands.length; i++) { | ||
| const { start, end, color } = bands[i]; | ||
| if (start <= value && value < end) { | ||
| return color; | ||
| } | ||
| } | ||
| return TRANSPARENT_COLOR; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is great for this PR and it's unlikely that there'll be hundreds of bands, though we could at some point harness our monotonicHillClimb (or derivative) for O(log n) as well as avoidance of a hand rolled loop
There was a problem hiding this comment.
yep definitely, this should anyway be exported, improved and reused as band/categorical scale if we want
| export const getColorScale = createCustomCachedSelector([getHeatmapSpecSelector], (spec): { | ||
| scale: ColorScale; | ||
| bands: Required<ColorBand>[]; | ||
| } => getBandsColorScale(spec.colorScale)); |
There was a problem hiding this comment.
Ah okay you handle these together in the selector, that's why you wanted to bundle them
# [34.1.0](v34.0.0...v34.1.0) (2021-08-19) ### Bug Fixes * **goal:** tooltip actual color ([#1302](#1302)) ([dbe9d36](dbe9d36)) * **heatmap:** improve legend item ([#1317](#1317)) ([49c35ce](49c35ce)) * **legend:** no truncation with single value ([#1316](#1316)) ([7ec8a9f](7ec8a9f)) ### Features * **goal:** optional target tick ([#1301](#1301)) ([88adf22](88adf22))
|
🎉 This PR is included in version 34.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
The heatmap legend now shows the full ranges configured, to improve its readability when using a categorical/band scale.
BREAKING CHANGE
The
<Heatmap>API is changed: the color scale configuration is self-contained into thecolorScaleprop. Therangesandcolorsare now described asColorBandobjects, where acoloris associated with astartandendvalues.The heatmap is currently marked as
@alphaso the breaking change will not be propagated as a major version bump.Details
colors,rangesandcolorScaleprops are now replaced byChecklist
packages/charts/src/index.ts(and stories only import from../srcexcept for test data & storybook)