refactor(color): compute continuous color domain via series plugins#642
Conversation
|
📖 Docs Preview is ready. |
|
Preview is ready. |
|
Visual Tests Report is ready. |
| * Called by `getDomainForContinuousColorScale` over the raw series data (before shape data is prepared). | ||
| * Omit for types that do not support a continuous color scale (e.g. treemap, sankey, radar). | ||
| */ | ||
| getColorValue?(data: T['data'][number]): number; |
There was a problem hiding this comment.
It might be beneficial to group these methods into logical blocks, similar to how it's done with the tooltip. Since the list of functions is growing, adding a bit of structure would prevent the flat list from becoming difficult to read.
| @@ -0,0 +1,129 @@ | |||
| // Populate the series registry so getSeriesPlugin works (see plugin registration gotcha). | |||
There was a problem hiding this comment.
Do we have to write this for all tests? Can you put it in a general setup?
|
|
||
| type Series = ChartData['series']['data']; | ||
|
|
||
| describe('utils/color/getDomainForContinuousColorScale', () => { |
There was a problem hiding this comment.
It might be better to avoid tying test names to file paths. When directories change, keeping test names in sync can be easily overlooked, which might lead to outdated or confusing test names in the future.
| validateXYSeries({series, xAxis, yAxis}); | ||
| validateStacking({series}); | ||
| }, | ||
| getColorValue: (d) => Number(d.x), |
There was a problem hiding this comment.
If colorValue is strictly meant to be a number, it might be better practice to handle the type casting outside the plugin closure rather than inside it.
… share test plugin registration
| /** Renders shapes into the provided SVG `<g>` element using D3. May return a cleanup function. */ | ||
| renderShapes(args: RenderShapesArgs): (() => void) | void; | ||
|
|
||
| // --- Tooltip --- |
There was a problem hiding this comment.
Why are there these comments? In the api, it looks more like something unhealthy. Everything should be clear/well-grouped by object keys
3 participants
Description
getDomainForContinuousColorScaleswitched over the series type, so a new typemeant editing
core/utils/color.ts. The per-point color value now comes from thetype's own plugin. Part of the ongoing pluginization effort.
What changed
SeriesPlugin: optionalgetColorValue?(data). Core delegates viagetSeriesPlugin(type).getColorValue?.(...)instead of switching.y(line/area/bar-x/waterfall/scatter),
x(bar-y),value(pie/heatmap/funnel),|x1 - x0|(x-range). treemap/sankey/radar keep no method and still throw.import '../../../plugins').Behavior
Unchanged — same mapping, same
[min, max], same error for unsupported types.