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.

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

Conversation

alexfauquette
Copy link
Member

Extracted from #13209

The idea is to put all TS definitions related to the series in the config interface, such that the pro package could define config for other series which will then be derived into other types.

@alexfauquette alexfauquette added typescript component: charts This is the name of the generic UI component, not the React module! labels Jun 4, 2024
@mui-bot
Copy link

mui-bot commented Jun 4, 2024

Deploy preview: https://deploy-preview-13366--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c67ff63

>;

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

@@ -9,7 +9,7 @@ import { ChartSeriesDefaultized, ChartSeriesType } from '../models/seriesType/co
import { AxisDefaultized } from '../models/axis';
import { ChartsTooltipClasses } from './chartsTooltipClasses';
import { DefaultChartsAxisTooltipContent } from './DefaultChartsAxisTooltipContent';
import { isCartesianSeriesType } from './utils';
import { isCartesianSeriesType } from '../internals/isCaterisan';
Copy link
Member

Choose a reason for hiding this comment

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

File name is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Which one would you recomand?

I preferred to keep a single file for both isCartesianSeries and isCartesianSeriesType so I picked the common part that is not the exact name of a function

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I don't mind much, the issue is that isCaterisan is the wrong spelling. Should be isCartesian

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 🙏

I'm somewhat insensitive to letter order, if I don't focus on a specific word

packages/x-charts/src/internals/configInit.ts Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 5, 2024
Copy link

github-actions bot commented Jun 5, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 5, 2024
@alexfauquette alexfauquette enabled auto-merge (squash) June 5, 2024 11:51
@alexfauquette alexfauquette merged commit 8c2de87 into mui:master Jun 5, 2024
15 checks passed
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants