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] Use plugins to define series extremum and colors #13397

Merged
merged 8 commits into from
Jun 7, 2024

Conversation

alexfauquette
Copy link
Member

The extremum commit is straightforward. Whereas, the color one required to add a context.

We currently use a function that takes the series and the different axes config and outputs a getter dataINdex => color.

This function is used at multiple places. So I'm introducing a provider to still be able to share it with all items

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

mui-bot commented Jun 6, 2024

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

Generated by 🚫 dangerJS against f3d28ed

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

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

Looks nice, added a suggestion based on my finding on the other pr.

Comment on lines 6 to 10
export function useColorProcessor<T extends ChartSeriesType>(
seriesType: T,
): ColorProcessorsConfig<ChartSeriesType>[T];
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>;
export function useColorProcessor(seriesType?: ChartSeriesType) {
Copy link
Member

Choose a reason for hiding this comment

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

Following #13396

We could allow ColorProcessor to trim down the correct types itself

// plugin.ts
export type ColorProcessor<T extends ChartSeriesType> = T extends ChartSeriesType
  ? (
      series: DefaultizedSeriesType<T>,
      xAxis?: AxisDefaultized,
      yAxis?: AxisDefaultized,
      zAxis?: ZAxisDefaultized,
    ) => (dataIndex: number) => string
  : never;

And then on this file

Suggested change
export function useColorProcessor<T extends ChartSeriesType>(
seriesType: T,
): ColorProcessorsConfig<ChartSeriesType>[T];
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>;
export function useColorProcessor(seriesType?: ChartSeriesType) {
export function useColorProcessor<T extends ChartSeriesType>(seriesType: T): ColorProcessor<T>;
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>;
export function useColorProcessor(seriesType?: ChartSeriesType) {

Comment on lines +1 to +26
// Components
export * from './components/ChartsAxesGradients';

// hooks
export { useReducedMotion } from '../hooks/useReducedMotion';
export { useSeries } from '../hooks/useSeries';

// utils
export * from './defaultizeValueFormatter';
export * from './configInit';

// contexts

export * from '../context/CartesianContextProvider';
export * from '../context/DrawingProvider';
export * from '../context/InteractionProvider';
export * from '../context/SeriesContextProvider';
export * from '../context/ZAxisContextProvider';

// series configuration
export * from '../models/seriesType/config';
export * from '../models/seriesType/common';

export * from '../models/helpers';
export * from '../models/z-axis';
export * from '../models/axis';
Copy link
Member

Choose a reason for hiding this comment

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

This file will probably cause a lot of unnecessary cyclic deps 😓

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you think so?

The internal/index.ts is exporting all the elements from the community package we need for creating the pro one but don't want to expose to the users.

  • they are not documented in API pages
  • they can only be imported with from '@mui/x-charts/internal which is assumed to be enough to know those API are to be considered unstable

So the community package has 3 kinds of elements:

  • Those publically exported by @mui/x-charts and @mui/x-charts/Xxxx (in blue)
  • Those privately exported by @mui/x-charts/internals (in red)
  • Those not exported (in yellow)

image

Since import/export is only going from MIT to pro, this should not create any loop. But I might miss something

Copy link
Member

Choose a reason for hiding this comment

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

Internals itself is a folder with content, and it is exporting files and folders outside of itself, on the same level.

Eg: export * from '../context/CartesianContextProvider';

The ../context is at the same level as ../internals

If on the CartesianContextProvider.tsx file, we remove import { SeriesId } from '../models/seriesType/common'; and reimport it, it will prefer the to import from ../internals and will cause a cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

By convention, we never import from /internals. You can import from internals/... but not the index file. This index file is only here to be used by the pro packages. THis convention should prevent that kind of issue

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be useful to have an eslint rule about it then?

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 never got any issues with that in 3 years 😇

Copy link
Member

Choose a reason for hiding this comment

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

🤔 my auto import creates cyclic deps instantly with these changes, then I have to manually change the import. But we can look at that in another MR

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants