feat(heatmap): move onBrushEnd from config to Settings

[rshen91]

Summary

BREAKING CHANGE

The onBrushEnd prop is removed from the heatmap config. It can now be pulled off the Setting to match the other chart types.

Details

In settings.tsx I created a new type BrushArea:

/** @public */
export type BrushArea = XYBrushArea | HeatmapBrushEvent;

/** @public */
export type BrushEndListener = (brushArea: BrushArea) => void;

This satisfies chart_state.interactions.test.tsx and on_brushed_highlighted_shapes.test.ts

Issues

Closes #1214

Checklist

• The proper chart type label has been added (e.g. :xy, :partition)
• All related issues have been linked (i.e. closes #123, fixes #123)
• New public API exports have been added to packages/charts/src/index.ts
• Unit tests have been added or updated to match the most common scenarios
• The proper documentation and/or storybook story has been added or updated

[markov00]

@nickofthyme @monfera @rshen91
I'd like to have your opinion here: this is not the only place where we have an union type as an argument of a chart callback.
It happens on the onClick event, but also in other situations.
Most chart consumers automatically cast to the right type depending on their needs: e.g. I'm using a bar chart so I'm cast with as XYBrushArea the brushArea.
Right now we don't offer a Generic type for the chart and I also don't fully know if and how this can work.
I'm wondering if we can, in the meantime, find a simpler and cleaner solution to the problem.
Tagging the callback argument type with the chart type could be a solution:

type HeatmapBrushEvent = {
  type: 'heatmap';
  cells: Cell[];
  x: (string | number)[];
  y: (string | number)[];
};

interface XYBrushArea {
  type: 'cartesian';
  x?: [number, number];
  y?: Array<GroupBrushExtent>;
}

what do you think?

[nickofthyme]

So I was thinking one way to do this would be in the function somehow like this...

<Settings
  onBrushEnd={('heatmap', event: HeatmapBrushEvent) => null}
  // or with multiple
  onBrushEnd={(['line', 'area'], event: LineBrushEvent | AreaBrushEvent) => null}
/>

The issue with this is that they could pass 'heatmap when there is no HeatmapSpec defined.

<Settings
  onBrushEnd={{
    types: ['heatmap'],
    callback: (event: HeatmapBrushEvent) => null
  }}
/>

The idea behind the types is something like below... (see TS Playground)

Screen.Recording.2021-09-14.at.08.38.04.AM.mp4

[nickofthyme]

I found a slightly simpler way to avoid the type recursion. See demo

interface HeatmapEvent {
    type: "heatmap"
    getHeatmap: () => {}
}

interface XYEvent {
    type: "xy"
    getXY: () => {}
}
interface EventMap {
    'heatmap': HeatmapEvent;
    'xy': XYEvent;
}

// There may be a less verbose way of doing this...
type Callback<Type> = Type extends keyof EventMap ?
    EventMap[Type] : never

type HeatmapCb = Callback<'heatmap'>; // HeatmapEvent
type XYCb = Callback<'xy'>; // XYEvent
type AnyCb = Callback<'xy'|'heatmap'>; // HeatmapEvent | XYEvent

[nickofthyme]

After looking at how we could implement this in the brush end listener, it is not currently easy to use a generic via Settings props. @rshen91 is opening an issue (#1380) to discuss a future enhancement.

[nickofthyme]

So I was thinking one way to do this would be in the function somehow like this...

<Settings
  onBrushEnd={('heatmap', event: HeatmapBrushEvent) => null}
  // or with multiple
  onBrushEnd={(['line', 'area'], event: LineBrushEvent | AreaBrushEvent) => null}
/>

The issue with this is that they could pass 'heatmap when there is no HeatmapSpec defined.

<Settings
  onBrushEnd={{
    types: ['heatmap'],
    callback: (event: HeatmapBrushEvent) => null
  }}
/>

The idea behind the types is something like below... (see TS Playground)

Screen.Recording.2021-09-14.at.08.38.04.AM.mp4

[nickofthyme]

Ugh sorry for got about git checkout would also wipe any auto changes in between. I just git reset to the good pr with the unit test fix on top 🍒 and force pushed 🙊

[markov00]

jenkins test this please

[nickofthyme]

LGTM! 🎉