Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[charts] Harmonize charts TS #13366

Merged
merged 4 commits into from
Jun 5, 2024
Merged

[charts] Harmonize charts TS #13366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd76f19
[charts] Harmonize charts TS
alexfauquette Jun 4, 2024
3b3636b
use singleton for config
alexfauquette Jun 5, 2024
e7297a5
fix file naming
alexfauquette Jun 5, 2024
c67ff63
Merge remote-tracking branch 'upstream/master' into ts-simplification
alexfauquette Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/x-charts/src/ChartsLegend/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export type AnchorPosition = { horizontal: AnchorX; vertical: AnchorY };

export type Direction = 'row' | 'column';

const legendGetter: { [T in ChartSeriesType]: LegendGetter<T> } = {
const legendGetter: { [T in ChartSeriesType]?: LegendGetter<T> } = {
bar: getBarLegend,
scatter: getScatterLegend,
line: getLineLegend,
Expand All @@ -21,7 +21,9 @@ const legendGetter: { [T in ChartSeriesType]: LegendGetter<T> } = {

export function getSeriesToDisplay(series: FormattedSeries) {
return (Object.keys(series) as ChartSeriesType[]).flatMap(
<T extends ChartSeriesType>(seriesType: T) =>
legendGetter[seriesType as T](series[seriesType as T]!),
<T extends ChartSeriesType>(seriesType: T) => {
const getter = legendGetter[seriesType as T];
return getter === undefined ? [] : getter(series[seriesType as T]!);
},
);
}
43 changes: 38 additions & 5 deletions packages/x-charts/src/models/seriesType/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,68 @@ import { DefaultizedProps, MakeOptional } from '../helpers';
import { StackingGroupsType } from '../../internals/stackSeries';
import { SeriesId } from './common';

interface ChartsSeriesConfig {
export interface ChartsSeriesConfig {
bar: {
/**
* Series type when passed to the formatter (some ids are defaultised to simplify the DX)
*/
seriesInput: DefaultizedProps<BarSeriesType, 'id'> & { color: string };
/**
* Series type when stored in the context (with all the preprocessing added))
*/
series: DefaultizedBarSeriesType;
canBeStacked: true;
/**
* Series typing such that the one user need to provide
*/
seriesProp: BarSeriesType;
itemIdentifier: BarItemIdentifier;
canBeStacked: true;
cartesian: true;
};
line: {
seriesInput: DefaultizedProps<LineSeriesType, 'id'> & { color: string };
series: DefaultizedLineSeriesType;
canBeStacked: true;
seriesProp: LineSeriesType;
itemIdentifier: LineItemIdentifier;
canBeStacked: true;
cartesian: true;
};
scatter: {
seriesInput: DefaultizedProps<ScatterSeriesType, 'id'> & { color: string };
series: DefaultizedScatterSeriesType;
seriesProp: ScatterSeriesType;
itemIdentifier: ScatterItemIdentifier;
cartesian: true;
};
pie: {
seriesInput: Omit<DefaultizedProps<PieSeriesType, 'id'>, 'data'> & {
data: (MakeOptional<PieValueType, 'id'> & { color: string })[];
};
series: DefaultizedPieSeriesType;
seriesProp: PieSeriesType<MakeOptional<PieValueType, 'id'>>;
itemIdentifier: PieItemIdentifier;
};
}

export type CartesianChartSeriesType = 'bar' | 'line' | 'scatter';
export type ChartSeriesType = 'bar' | 'line' | 'scatter' | 'pie';
export type ChartSeriesType = keyof ChartsSeriesConfig;

export type CartesianChartSeriesType = keyof Pick<
ChartsSeriesConfig,
{
[Key in ChartSeriesType]: ChartsSeriesConfig[Key] extends { cartesian: true } ? Key : never;
}[ChartSeriesType]
>;

export function isCartesianSeriesType(seriesType: string): seriesType is CartesianChartSeriesType {
return ['bar', 'line', 'scatter', 'heatmap'].includes(seriesType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a singleton where we can register different types of cartesian series? something like

class CartesianSeriesTypes {
  _types: Set<ChartSeriesType> = new Set(['bar', 'line', 'scatter'])

  constructor() {
    if (CartesianSeriesTypes._instance) {
      return CartesianSeriesTypes._instance
    }
    CartesianSeriesTypes._instance = this;
  }

  addType(value: 'string') { _types.add(value) }
  getTypes() { return _types }
}

this would allow us to register new types from other packages. We could also make the class a bit more generic and instead allow registering full configurations, eg: SeriesTypes.register({ name: "heatmap", formatter: (v) => v.foo, isCartesian: true })

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you would instantiate it in the index.ts of each package.

Can be an elegant solution for the isCartesianSeriesType. I'm not fully in favor of expanding this strategy to other aspect, because one advantage of plugins is the tree shaking. Since the heatmap logic is only imported in tht src/Heatmap/ folder, if you don't import it you will not bundle its logic. Same for the tree view, radar, and other upcoming charts. Not sure it will be the same with this approach

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we could only register if the user imports charts/heatmap or something like that.

I've been thinking about the plugin, and the drawback might be that we will need to pass the configuration around to everything that would need it 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the drawback might be that we will need to pass the configuration around to everything that would need it 😅

We only need to add them in the single component definition:

// In src/Heatmap/heatmap.tsx
<ChartsContainer plugins={[heatmpaPlugin]} > ...

If user do composition they will have to do the same thing when defining their component.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the modification for the config. I did not used it in the pro package yet, because otherwise TS will complain that 'heatmap' is not a series type yet. Will be added in the next PR

}

export type StackableChartSeriesType = keyof Pick<
ChartsSeriesConfig,
{
[Key in ChartSeriesType]: ChartsSeriesConfig[Key] extends { canBeStacked: true } ? Key : never;
}[ChartSeriesType]
>;

export type ChartSeries<T extends ChartSeriesType> = ChartsSeriesConfig[T] extends {
canBeStacked: true;
Expand Down
44 changes: 22 additions & 22 deletions packages/x-charts/src/models/seriesType/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
import { BarItemIdentifier, BarSeriesType, DefaultizedBarSeriesType } from './bar';
import { DefaultizedLineSeriesType, LineItemIdentifier, LineSeriesType } from './line';
import { DefaultizedScatterSeriesType, ScatterItemIdentifier, ScatterSeriesType } from './scatter';
import { DefaultizedPieSeriesType, PieSeriesType, PieItemIdentifier, PieValueType } from './pie';
import { MakeOptional } from '../helpers';
import { BarSeriesType, DefaultizedBarSeriesType } from './bar';
import {
CartesianChartSeriesType,
ChartSeriesType,
ChartsSeriesConfig,
StackableChartSeriesType,
} from './config';

type AllSeriesType =
| BarSeriesType
| LineSeriesType
| ScatterSeriesType
| PieSeriesType<MakeOptional<PieValueType, 'id'>>;
// Series definition

type CartesianSeriesType = BarSeriesType | LineSeriesType | ScatterSeriesType;
type AllSeriesType<T extends ChartSeriesType = ChartSeriesType> =
ChartsSeriesConfig[T]['seriesProp'];

type DefaultizedCartesianSeriesType =
| DefaultizedBarSeriesType
| DefaultizedLineSeriesType
| DefaultizedScatterSeriesType;
type CartesianSeriesType = AllSeriesType<CartesianChartSeriesType>;

type DefaultizedSeriesType = DefaultizedCartesianSeriesType | DefaultizedPieSeriesType;
type DefaultizedSeriesType<T extends ChartSeriesType = ChartSeriesType> =
ChartsSeriesConfig[T]['series'];

type StackableSeriesType = DefaultizedBarSeriesType | DefaultizedLineSeriesType;
type DefaultizedCartesianSeriesType = DefaultizedSeriesType<CartesianChartSeriesType>;

export type SeriesItemIdentifier =
| BarItemIdentifier
| LineItemIdentifier
| ScatterItemIdentifier
| PieItemIdentifier;
type StackableSeriesType = DefaultizedSeriesType<StackableChartSeriesType>;

// item identifier

export type SeriesItemIdentifier<T extends ChartSeriesType = ChartSeriesType> =
ChartsSeriesConfig[T]['itemIdentifier'];

export * from './line';
export * from './bar';
Expand All @@ -39,6 +37,8 @@ export type {
StackableSeriesType,
};

// Helpers

export function isDefaultizedBarSeries(
series: DefaultizedSeriesType,
): series is DefaultizedBarSeriesType {
Expand Down
Loading