Conversation
This prop was passed in, but never actually given to ECharts, so it was pointless!
Fix Zstep range calculation (was Zmin+Zmax, should be Zmax-Zmin), remove duplicate PlottableTimeSeriesValueType from heatMap.tsx, and remove unused LOADING_PLACEHOLDER export from settings. Refs LOGS-762 Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.45% |
These files aren't consumed by any production entry point yet. Add a temporary knip entry point exception until the widget is integrated into Explore. Refs LOGS-762 Co-Authored-By: Claude <noreply@anthropic.com>
HeatMapValueType is only used internally within types.tsx, so it doesn't need to be exported. Fixes knip unused-export violation. Refs LOGS-762 Co-Authored-By: Claude <noreply@anthropic.com>
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Replace the piecewise color mapping with a two-visualMap setup: a piecewise map that makes z=0 buckets fully transparent via opacity, and a continuous map that interpolates across HEATMAP_COLORS for all non-zero values. This avoids hardcoded step boundaries and produces smooth color gradients. Refs LOGS-762 Co-Authored-By: Claude <noreply@anthropic.com>
Pass scale through to the plottable via plotting options instead of post-processing series data in the visualization component. The HeatMap class now handles the log1p transform in toSeries when scale is 'log', keeping data transformation in the plottable layer where it belongs. Refs LOGS-762 Co-Authored-By: Claude <noreply@anthropic.com>
…-762-create-heatmapwidgetvisualization-component
Remove the second huge fixture, it's not that useful.
The `visualMap` prop on BaseChart was placed after the `...options` spread, meaning an undefined `visualMap` would overwrite any `visualMap` set through `options`. Pass it through `options` instead. Co-Authored-By: Claude <noreply@anthropic.com>
|
@cursor review |
The piecewise visualMap had overlapping pieces at value 0: one piece targeted exactly 0 with opacity 0, while another used min: 0 (>=0) with opacity 1. ECharts has inconsistent behavior with overlapping piece boundaries, which could cause zero-value cells to render opaque. Using gt: 0 (strictly greater than) removes the ambiguity.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b1d4f84. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
nsdeschenes
left a comment
nsdeschenes
left a comment
There was a problem hiding this comment.
Some quick questions, but everything is looking good 👍
There was a problem hiding this comment.
True but that's c'est-la vie for ya
| /** | ||
| * Experimental! Specify the Z-axis scale type. Logarithmic scales can be much more useful for values with a high range. | ||
| */ | ||
| scale?: 'linear' | 'log'; |
There was a problem hiding this comment.
Just wondering how this PR, impacts/interacts with this?
If we enable one but not the other, would it cause a broken UX?
There was a problem hiding this comment.
Great question! The backend PR adds support for log yAxis which has to be done on the backend, and this is experimental support for log zAxis which I think is easier to do on the frontend, but I'm not sure, it was just faster to do it here as a test
| return [ | ||
| item.xAxis, | ||
| item.yAxis, | ||
| scale === 'log' && typeof zValue === 'number' ? Math.log1p(zValue) : zValue, |
There was a problem hiding this comment.
We seem to be doing log stuff on the API side, if I'm understanding this PR right.
Would we be better off migrating the calculations that are happening here to be on the API side? So we don't have a mismatch between datasets 👀
There was a problem hiding this comment.
I think that might be better on the API side but I'm not confident yet, and it's easier to iterate on this on the frontend for now. There's a decent chance we don't even use log, but something else instead, I have a whole ticket to take a look at this problem space
The tuple type [HeatMap, ...HeatMapPlottable[]] already guarantees the first element is a HeatMap, so the runtime .find() + non-null assertion was unnecessary.
BaseChart uses echarts/core (tree-shakeable) and only registers grid, graphic, toolbox, and brush. The heatmap chart type and visualMap component were only available because the old HeatMapChart in tagsHeatMap.tsx registers them globally as a side effect. Adding explicit imports removes this latent dependency.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a5acd27. Configure here.
gggritso
deleted the
georgegritsouk/logs-762-create-heatmapwidgetvisualization-component
branch
3 participants
Before having Heat Map support in Explore, one must have a Heat Map component to put into Explore! This is that widget. This is an MVP, it doesn't even have any features aside from plotting that mostly works. What it does have is:
e.g.,
This component follows a very similar structure to how the time series, and categorical visualization widgets work. They declare a
Plottableinterface that the visualization component can plot, and each plottable kind (in this case, justHeatMap) works against that interface. There's a decent chance that we'll support other heat map plottables if we want to show percentiles on the same chart.This is working from @marthalyndon's generous #114616 which spiked this feature a while back. Thank you Martha!
I also had to update
BaseChartbecause it was dropping itsvisualMapprop on the floor mysteriously!The
BaseChartcomponent is extended with avisualMapprop passthrough since ECharts heat maps require a visual map for color mapping. TheHeatMapPlottableinterface is intentionally separate from the existingPlottableinterface since heat maps have fundamentally different axes (X, Y, Z) compared to time series (X, Y).This is pretty raw for now, we'll be coming back to think through the colour palette and so on.
Closes LOGS-762